Classloader leak via CacheImpl.invocationContextContainer

[brian.stansberry]

The handling of InvocationContext will leak JBC's classloader. An InvocationContext gets stored in a ThreadLocal and doesn't get cleared. This is by design to avoid creating a new InvocationContext for each request, but it will have the effect of leaking the JBC classloader to whatever threads call into JBC.

Note that the fact that CacheImpl.invocationContextContainer is an instance field will not prevent the leak. When the CacheImpl is gc'd, the invocationContextContainer ThreadLocal will be gc'd as well. However, for each thread that invoked on the cache there will still be a ThreadLocal.ThreadLocalMap.Entry whose value field is a strong ref to the InvocationContext. If a particular thread's ThreadLocalMap happens to do some cleanup work (i.e. a rehash) it will detect that that ThreadLocal has been gc'd and expunge the Entry, but to allow the classloader to be gc'ed all such threads would have to do that, which is unlikely.


Solving this will be a pain, since clearing the ThreadLocal after each request will likely break a number of subtle things where code calls back into the cache. For example, Notifier would probably have to be changed.

[jason.greene]

So why don't we make it a weak ref, and recreate it if its null?

[brian.stansberry]

Yeah, that should work. We pass the context as a param to Interceptor.invoke now, so there is no chance of it getting lost in the middle of a call.

Guess solving it's not such a pain. :) Thanks.

[brian.stansberry]

Hmm, yeah there is a chance of losing it...

cache.getInvocationContext().getOptionOverrides().setCacheModelLocal(true);

// gc runs

cache.put(fqn, key, value);


Call gets replicated.

[jason.greene]

Ah yes. Perhaps just clearing it is better. Although then it wouldn't survive calls.

cache.getInvocationContext().getOptionOverrides().setCacheModelLocal(true);
cache.put(fqn, key, value);
cache.put(fqn, key, value);

Another option would be to have CacheImpl store a strong reference to every known context, which would go away on cleanup, although this would probably be expensive (at least for creation of the context)

Last, we could require that calling code hold the reference (somewhat cryptic, but this is an advanced feature).

InvocationContext ic = cache.getInvocationContext();
ic.getOptionOverrides().setCacheModelLocal(true);
cache.put(fqn, key, value);
cache.put(fqn, key, value);

-Jason

[manik]

I think the InvocationContextInterceptor should clean up the thread local entry as the invocation exits.

The only place this causes a problem is if there is a callback (like you said, with the notifier) and this triggers another cache operation which ends up resetting the thread local entry. This is less of a problem now since interceptors don't rely on the thread local once the call goes past the ICI (the IC is passed as a param). It is also less of a problem since (JBCACHE-1022) all notifications will now be emitted from the NotificationInterceptor, after which there is not much more than cleanup work remaining.

I think having the ICI clean up thread locals explicitly is more robust and maintainable than weak refs or calling code holding on to refs.

[jason.greene]

I know we are too late in the game for API changes but...

If the InvocationContext is only available for one call, then does it really make sense to expose this as a public API? Wouldn't it be more sensible to make this part of CacheSPI?

There are a lot of options here that would be pretty hazardous to mess with.

-Jason

[brian.stansberry]

There absolutely needs to be some mechanism for clients to set Option flags on calls via the Cache API. Pretty much all AS uses of JBC would fail w/o that.

Agreed though that InvocationContext exposes more than a client should be able to tweak.

[manik]

Needed by Hibernate providers to inject their own DataVersion implementations for optimistically locked nodes, where versioning is maintained elsewhere (e.g., in a DB).

Among other things.

[brian.stansberry]

That's via the Option too, right?

All the stuff in Option is appropriate for a client API, except I guess failSilently which AIUI is an implementation detail of putForExternalRead.

How about using interfaces for this -- InvocationContext and InvocationContextSPI, plus InvocationContextImpl? The InvocationContext interface just exposes the getters that make sense for a client, and via getOptionOverrides() makes the Option available.

If getOptionOverrides() is the only thing a client InvocationContext interface should expose, then maybe it makes more sense to change Cache so it exposes Cache.getOptionOverride() rather than getInvocationContext(). CacheSPI can expose getInvocationContext().

[jason.greene]

If we do this, we could keep options around for ever (no CL leak) by having the Options object delegate to a hashmap on the threadlocal.

[brian.stansberry]

Does keeping any of the options around beyond the life of a single invocation make sense? When I look at them they all seem like one-time-only settings.

[jason.greene]

IMO, if they shouldn't survive multiple invocations, then they should just be a parameter, else its really alien to general java usage.

cache.put(fqn, key, object, options);

-Jason

[brian.stansberry]

No argument there. That's how it is in the 1.x API, but the overloaded methods got cut in a drive to trim the 2.0 API.

[jason.greene]

A one shot option that is not associated with a method is error prone and verbose for calling code.

Consider this code

cache.getOptions().setForceWriteLock(true);
String atomicUserCounter = cache.get("/user/123", "counter");

Later someone assumes this is a persisting option and updates it

cache.getOptions().setForceWriteLock(true);
String atomicUserCounter = cache.get("/user/123", "counter");
String atomicGroupCounter = cache.get("/group/456", "counter");

The fix is overly verbose:

cache.getOptions().setForceWriteLock(true);
String atomicUserCounter = cache.get("/user/123", "counter");
cache.getOptions().setForceWriteLock(true);
String atomicGroupCounter = cache.get("/group/456", "counter");

That said persistent options could lead to problems as well.

An overloaded parameter is neither error prone, nor overly verbose, and is intuitive:

Options options = new Options();
options.setForceWriteLock(true);
String atomicUserCounter = cache.get("/user/123", "counter", options);
String atomicGroupCounter = cache.get("/group/456", "counter", options);
String regularReadValue = cache.get("/foo", "foo");

You could even offer constant options (via an immutable extension):

String atomicUserCounter = cache.get("/user/123", "counter", Options.FORCE_WRITE_LOCK);
String atomicGroupCounter = cache.get("/group/456", "counter", Options.FORCE_WRITE_LOCK);
String regularReadValue = cache.get("/foo", "foo");

-Jason