Issue #6752 DefaultSessionCache more extensible using ConcurrentMap

[prenagha]

DefaultSessionCache is designed to be extended, by virtue of its protected session map. Subclasses can set their own map instance instead. However the session map is specified as ConcurrentHashMap, when it only needs to be ConcurrentMap.
Changed data type to ConcurrentMap to allow for wider options for subclasses, such as those wanted to use Caffeine's asMap() method which returns ConcurrentMap.
Although changing to even more relaxed Map would work, that does not provide as much clarity that the map will be used concurrently - therefore used ConcurrentMap instead.

Resolves #6752

Signed-off-by: Padraic Renaghan padraic@renaghan.com

[prenagha]

Presumably, if accepted, you'll want this change on 11.0.x branch as well. Let me know if that happens automatically or if you want me to open separate issue/PR for the 11.0.x branch.

[prenagha]

Here is an example of how I am trying to extend

public class SessionExpiringNearCache extends DefaultSessionCache {

  private final Cache<String, Session> nearCache =
    Caffeine.newBuilder()
    .expireAfterAccess(15, TimeUnit.MINUTES)
    .build();

  public SessionNearCache(SessionHandler manager) {
    super(manager);
    this._sessions = nearCache.asMap();
  }
}

[gregw]

@prenagha Typically we just merge forward changes from 10 to 11.

[gregw]

LGTM. Thanks

[janbartel]

@prenagha I don't have any problem with a ConcurrentMap instead of a ConcurrentHashMap.

However, are you sure you want to be subclassing the DefaultSessionCache, and that what you're not looking for is actually making an L2 cache for session data https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-server-session-cachingsessiondatastore? Alternatively, there's a lot of different expiry behaviour you can configure on the DefaultSessionCache without necessarily subclassing, eg: https://www.eclipse.org/jetty/javadoc/jetty-10/org/eclipse/jetty/server/session/AbstractSessionCache.html#setEvictionPolicy(int). That said, you are more than welcome to extend the DefaultSessionCache any way you want, I just want to make sure you've considered all the stuff that's available out-of-the-box ;)

[joakime]

I don't like the example usage of calling this._sessions = .....

I think I would have preferred the API to set the map as a constructor.

ala.

public class SessionExpiringNearCache extends DefaultSessionCache {

  private final Cache<String, Session> nearCache =
    Caffeine.newBuilder()
    .expireAfterAccess(15, TimeUnit.MINUTES)
    .build();

  public SessionNearCache(SessionHandler manager) {
    super(manager, nearCache.asMap());
  }
}

or as a DefaultSessionCache.getSessionsMap() method that could be overridden, and all internal uses of the map have to go through that method.

or at at the very least as a proper (protected) setter method DefaultSessionCache.setSessionsMap(ConcurrentMap impl)

[prenagha]

@janbartel thanks for the pointers on DefaultSessionCache doc, and I agree I likely will implement (or find existing) L2 cache using Redis backend.

Probably will end up using default for L1, but am experimenting with some additional options that Caffeine provides to see if useful.

For the L2 Redis backend, if you have a pointer to an existing implementation that is widely used let me know. Otherwise was going to use JDBCSessionDataStore as a guide/starting point.

[prenagha]

@joakime I agree that extensibility could be better done. However I was just trying to follow the lead of DefaultSessionCache which already has

protected ConcurrentHashMap<String, Session> _sessions = new ConcurrentHashMap<>();

protected presumably so can be overridden in subclasses with a simple assignment. At least that was what I guessed the intent was, and so I just tried to follow that lead here.

[joakime]

@joakime I agree that extensibility could be better done. However I was just trying to follow the lead of DefaultSessionCache which already has

protected ConcurrentHashMap<String, Session> _sessions = new ConcurrentHashMap<>();

protected presumably so can be overridden in subclasses with a simple assignment. At least that was what I guessed the intent was, and so I just tried to follow that lead here.

I just took a look at the code in jetty-10.0.x, nothing but the DefaultSessionCache is using that field.

[joakime]

@prenagha I tossed up an alternate proposal at PR #6758