CUSTCOM-135 Fixed race condition on ConfigProviderResolver initialization #4480
Conversation
…tion - instane() is synchronized - setInstance is NOT synchronized, so it ignores any locks - preferring @Inject over ConfigProvider in HealthCheckService - using @ContractsProvided to allow both @Inject target types - more logging, but only when configured - tested in extremely slowed TestContainers docker container
| if (resolver != null && resolver != this) { | ||
| return resolver; | ||
| } | ||
| Thread.yield(); |
There was a problem hiding this comment.
.yield() is a hint, which means that control isn't necessarily passed on to another Thread. A sleep with very small amount should be used.
There was a problem hiding this comment.
I used it on Linux+Intel/AMD and on Solaris/SPARC, even old versions, and it always worked (lower CPU load because it is not so aggressive as while loop, but fast, because it does not sleep).
Also it is only few time blocks after startup, so it does not slow down the Payara Startup. Sleep is unnecessary overkill, because I don't need to sleep running thread, I only tell the scheduler "at this moment I have nothing to do, but if other threads have some work, they can".
https://www.geeksforgeeks.org/java-concurrency-yield-sleep-and-join-methods/
There was a problem hiding this comment.
instance() also has synchronized block, so there's another yield point there, so I believe this might be correct.
But still, I trust CountDownLatch more, but might be overkill for 1 producer to 1 consumer scenario.
| // this will block possible concurrent access. | ||
| synchronized (ConfigProviderResolver.class) { | ||
| LOG.log(Level.CONFIG, "Setting global ConfigProviderResolver instance to {0}", this); | ||
| ConfigProviderResolver.setInstance(this); |
There was a problem hiding this comment.
This is executed too early if you ask me. The current instance is set as value BEFORE the instance is completely setup as HK2 service. So MP config calls can get routed to this class before HK2 is finished processing, resulting in errors (NPE since the @Inject isn't processed?)
There was a problem hiding this comment.
I agree, I was a bit too agressive and this is an antipattern (providing this before finishing constructor). But ... if we don't have other possibility, isn't the @PostConstruct method same situation?
From what I tested, @Inject will not cause NPE here, because HK2 will block any injection until it finishes PostConstruct method (constructor already finished). Bigger problem is programmatic usage of Globals class.
There was a problem hiding this comment.
Semantics of postconstruct guarantees that fields are set. The situation we're dealing with here is that someone triggered mp config API before this service was initialised. Postconstruct is therefore almost proper time as it is considered initialized after the method exits
Injections of this class via globals is indeed wrong. But it is used by config sources, and those are created by this service. Plain constructor parameter would be better here.
| if (resolver != null && resolver != this) { | ||
| return resolver; | ||
| } | ||
| Thread.yield(); |
There was a problem hiding this comment.
instance() also has synchronized block, so there's another yield point there, so I believe this might be correct.
But still, I trust CountDownLatch more, but might be overkill for 1 producer to 1 consumer scenario.
- safer, all fields should be set here.
|
Jenkins test please |
Important Info
Removed the timeout and countdouwn latch, because
Testing
New tests
Not pushed reproducer in PAYARA-4176 branch (not production quality, but stacktraces in logs visible)
Testing Performed