Fix a race condition that leads to ClassAnalyzer with name CdiInjecteeSkippingClassAnalyzer exceptions

[OndroMih]

This solves the race condition - keep the effective injectionManager in a thread local. As multiple threads can modify the injectionManager and the InjectionManagerInjectedCdiTarget instances are global, effectiveInjectionManager must be scoped to the thread. It's actually scoped to a single WebTarget initialization and removed at the end of the initialization in the done() method of CdiComponentProvider

The problem:

A race condition happens when many threads execute REST client call at the same (more specifically, when WebTarget.request() is called.

Symptom:

Occasional
IllegalStateException: Could not find an implementation of ClassAnalyzer with name CdiInjecteeSkippingClassAnalyzer from WebTarget.request

Reason for the exception:

• The injectionManager is set in the initialization phase, before it's used. It's stored to a shared volatile variable and later picked up
• If some thread sets the shared volatile variable to another injectionManager, before the original thread uses it, the original thread would use another thread's injectionManager
• In case the second thread doesn't bind the ClassAnalyzer class in time before the first thread uses it, the first thread won't have ClassAnalyzer bean available and throws exception

A reproducer is described in payara/Payara#7753.

The original fix in Payara fork (53ae843) addresses this by a workaround - a retry in case of exception, which works, because by the next time, the other thread has enough time to bind the ClassAnalyzer class. But that solution isn't efficient.

[OndroMih]

Hi, @jansupol , @Verdent , could you please review and approve if OK?

[jansupol]

@OndroMih
Would it be performance better (for many threads) if the ThreadLocal would be in each InjectionManagerInjectedCdiTarget?

[OndroMih]

@jansupol , I wanted to set the threadlocal in each target instance initially. But I didn’t find a nice way to access all targets from done method to remove the thread locals. Having the thread local in the top-level instance is easier because it can be accessed both from the done method and from all targets directly.

The targets are not directly accessible from their top-level class, if I’m not wrong. They are referenced from a store, which only supports setting injection manager. I would have to adjust the store contract to somehow allow removing the thread locals from all targets. Do you have an idea how to do it? Is it worth it?

[OndroMih]

@jansupol , maybe for a better performance, targets could set the thread local only if it's not set already. I read that ThreadLocal.get is faster than ThreadLocal.set, even if setting the same value that it already contains.

Within a single invocation, the injection manager would always be the same so with the current change, the first target sets the ThreadLocal value and all others just set it again to the same value. If they call get instead, it could be faster.

[OndroMih]

By trying that, I found out that the done method is not call on the instance of CdiComponentProvider which holds the targets. The done method is called on a completely different CdiComponentProvider, therefore a different thread local is removed, a one that contains null value and not an injectionManager.

It looks like CdiComponentProvider that contains the targets, is global to the application and shared by different REST client invocations.

I struggle to find out a good way to remove the thread local, I don't know how to get the correct instance of CdiComponentProvider in the done method, which is called from a REST client request.

[OndroMih]

I found the correct component that holds the thread local. It's the same extension which is used in the initialize method. Now the code removes the previously set thread local. And setting the thread local only if it's not already set also works.

[OndroMih]

A tested detected a leak - in some cases, ClientRuntime initializes CdiComponentProvider again, after postInit, without calling done later.

My solution is a bit hacky - move postInit for ClientComponentConfigurator to after ClientRuntime is created to properly clean up the thread local, while keep calling postInit for other configurators before ClientRuntime is created, because ClientRuntime depends on recourses set up in other configurators.

ClientComponentConfigurator only cleans up resources in postInit, so it's OK to call it later.

The initRuntime method already treats messageBodyWorkersConfigurator in a special way, so I think it's OK to add a special treatment also for ClientComponentConfigurator.

Another solution that also works would be to call postInit on ClientComponentConfigurator twice, at the same place as before, with all other configurator, and once again after ClientRuntime is created. But I didn't want to call it twice.

Yet another solution would be to add a special method to ClientComponentConfigurator, e.g. cleanup(), but that would introduce a few more changes - ClientComponentConfigurator delegates to multiple ComponentProvider instances. So I would need to add another default NoOp cleanup method to the ComponentProvider interface and another cleanup method to CdiComponentProvider to remove the thread local.

The code in ClientConfig would then look like before, with just another call to:

preInit.clientComponentConfigurator.cleanup(injectionManager, preInit.bootstrapBag);

Or if I also add a default NoOp method to BootstrapConfigurator interface, we could make it more general (the cleanup method would be called for all configurators, just as the postInit method is called earlier):

bootstrapConfigurators.forEach(configurator -> configurator.cleanup(injectionManager, preInit.bootstrapBag));

All of these solutions would work equally. The questions is which solution is better?

What do you think, @jansupol , @senivam ?

Is it better to treat the ClientComponentConfigurator specially and call postInit for it later than for other components? (my current solution)

Or is it better to call postInit as before, and add cleanup method to cleanup resources?