Issue #6752 - Extensible DefaultSessionCache map implementation

[joakime]

Alternate approach to PR #6753 from @prenagha

Closes #6752

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

[joakime]

@prenagha an additional constructor to this PR could be added to allow for some late construction of the map. But I'm not sure if it's worth the effort.

    /**
     * @param manager The SessionHandler related to this SessionCache
     * @param sessionMapSupplier The supplier of the session map implementation to use (to allow for late binding)
     */
    public DefaultSessionCache(SessionHandler manager, java.util.function.Supplier<ConcurrentMap<String,Session>> sessionMapSupplier)
    {
        this(manager, sessionMapSupplier.get());
    }

which would allow your example to work like ...

public class SessionExpiringNearCache extends DefaultSessionCache {
  public SessionNearCache(SessionHandler manager) {
    super(manager, () -> Caffeine.newBuilder()
        .expireAfterAccess(15, TimeUnit.MINUTES)
        .build());
  }
}

or just use the DefaultSessionCache directly, no extends ...

DefaultSessionCache sessionCache = new DefaultSessionCache(sessionHandler,
   () -> Caffeine.newBuilder().expireAfterAccess(15, TimeUnit.MINUTES).build());

or (if using jgroups for example)

DefaultSessionCache sessionCache = new DefaultSessionCache(sessionHandler, new ReplicatedHashMap<>(jgroupsChannel));

[prenagha]

@joakime Constructor approach looks good. Not sure if you all ever do null checking, but might be a good idea to add to the constructor that takes the map

Objects.requireNonNull(sessionMap, "Session Map is required");

[janbartel]

If you get rid of the "this", I'll approve this.

[janbartel]

Suggested change

	this._sessions = sessionMap;
	_sessions = sessionMap;

[gregw]

so whilst we are picking nits... the field and argument names should both be the same. either _sessions = sessions or _sessionMap = sessionMap. Since the field is protected, it is kind of part of the class contract, so I guess it can't be renamed.

[janbartel]

@joakime I'm definitely not interested in making the constructor any more complicated than just simply passing in an arg of type ConcurrentMap because it is just not worth it. But I am interested in either the addition of the Objects.requireNonNull check in the constructor, or a null check in the doStart() method (good catch @prenagha).

[gregw]

other option to avoid null issues is:

Suggested change

	{
	this(manager, new ConcurrentHashMap<>());
	}
	/**
	* @param manager The SessionHandler related to this SessionCache
	* @param sessionMap The session map implementation to use
	*/
	public DefaultSessionCache(SessionHandler manager, ConcurrentMap<String, Session> sessionMap)
	{
	this(manager, null);
	}
	/**
	* @param manager The SessionHandler related to this SessionCache
	* @param sessionMap The session map implementation to use
	*/
	protected DefaultSessionCache(SessionHandler manager, ConcurrentMap<String, Session> sessionMap)
	{
	super(manager);
	_sessions = sessionMap == null ? new ConcurrentHashMap() : sessionMap;

[gregw]

Also make this constructor protected

[janbartel]

@joakime is this ready for re-review yet?

[joakime]

how is this still conflicting??

[joakime]

@janbartel this PR is ready for a review

[janbartel]

This check shouldn't be necessary: _sessions is final && the constructor at line 51 ensures a non-null map && the other constructor at line 61 rejects nulls.

[joakime]

I just removed the entire doStart()