Check configuredProvider that it holds valid CDI container.

[doychin]

Check for both IllegalStateException and null value when filtering discovered providers.

Signed-off-by: Doychin Bondzhev doychin@dsoft-bg.com

[Emily-Jiang]

While executing JSON-B TCK, we hit this new code path. The extra check on CDIProvider.getCDI() returns null during the lazy initialization, which wipes out CDIProvider. I would like to find out the issue for this fix. @doychin can you point me to the original problem you are trying to fix? Thanks

[doychin]

This was the original problem that I found when I was trying to run unit tests in CDI container. Because all tests run in the same JVM and each one of them was bringing up separate container I hit the problem where original code in CDI and in weld was generating NPE.

https://issues.redhat.com/browse/WELD-2567

The problem comes from the fact that once container is initialized and then closed you can't get new one from original cdi provider. It will always return null and original code will not try to initialize new provider. It will just throw NPE.

[manovotn]

@Emily-Jiang Original issue was https://issues.redhat.com/browse/CDI-743 so take a look there for some more context.

If memory serves, the issue was that the CDI implementation doesn't behave the way it was described in the javadoc. As in, javadoc claims ISE could be thrown, but the code didn't expect that and would not search for other providers if an exception popped up.

EDIT: Weld 4.0.0.CR1 (and 4.0.0.Final) already contain the adjusted behavior for this, so you might want to upgrade to that if you aren't using it already.

[benjamin-confino]

Throwing an ISE here makes sense, however there is still something that I feel is wrong with this new code.

Line 82: configuredProvider = null; means that a provider that was set in setCDIProvider() will be nulled. Andit may be lost for good since if it is unlikely the integrator will be calling setCDIProvider() twice, or registering it in the ServiceLoader. The comments say you don't need to: If a {@link CDIProvider} is set using this method, any provider specified as a service provider will not be used.

This seems wrong to me. Just because a cdiProvider returns null once does not mean it will never return a valid result in the future. In Liberty we have one provider where getCDI() returns a different result depending on which application invokes CDI.current(). For deployments which are not CDI Enabled it will return null, with this update that will delete the reference to the CDIProvider, and prevent it from being used when a different CDI enabled deployment needs it.

I think we should add a flag that is set to true if configuredProvider was set in setCDIProvider(). When the flag is true we do not set configuredProvider to null or change it's value in getCDIProvider(). If configuredProvider.getCDI() returns null and the flag is true we throw the exception straight away.

[manovotn]

Just because a cdiProvider returns null once does not mean it will never return a valid result in the future.

Well, you cannot really know. It might just shut down that provider (its container) and would always return null from there on.

means that a provider that was set in setCDIProvider() will be nulled.

Hmm, I see what you mean. The current approach tries to re-initiate the the search for any other provider that works - an approach needed if you were to swap multiple providers on the fly (does anyone actually do that?).
But with manual provider setting, we lose that information and that's an issue.

I think we should add a flag that is set to true if configuredProvider was set in setCDIProvider(). When the flag is true we do not set configuredProvider to null or change it's value in getCDIProvider(). If configuredProvider.getCDI() returns null and the flag is true we throw the exception straight away.

Sounds good, we should add a boolean and also change the javadoc of setCDIProvider to indicate that in case the provider doesn't work, no other will be picked up anyway.
@benjamin-confino would you care to file a new issue and send PR?

[benjamin-confino]

I would be happy to create a PR.