Fix race condition in LanguageServerWrapper resulting in NullPointerException during LS initialization #985
Conversation
sebthom
force-pushed
the
npe
branch
2 times, most recently
from
dbac7c9 to
664f289
Compare
sebthom
requested review from
rubenporras and
mickaelistria
and removed request for
rubenporras
|
The tests failures are all related? |
|
No, we are getting random OOMs for builds since 26th of April https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-975/4/. No idea why. I opened a ticket but they say it must be a config change in our build process. |
When running the test suite on Windows a lot of NPEs with the following stacktrace are thrown: ``` java.lang.NullPointerException: Cannot invoke "org.eclipse.core.runtime.IProgressMonitor.done()" because "this.this$0.initializeFutureMonitor" is null at org.eclipse.lsp4e.LanguageServerWrapper$2.run(LanguageServerWrapper.java:407) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) ``` and because of the NPE random tests seem to fail.
|
@mickaelistria @rubenporras any objections to merge this? |
| return new Job(NLS.bind(Messages.initializeLanguageServer_job, serverDefinition.label)) { | ||
| @Override | ||
| protected IStatus run(IProgressMonitor monitor) { | ||
| initializeFutureMonitor = SubMonitor.convert(monitor, initializeFutureNumberOfStages); |
There was a problem hiding this comment.
Maybe it is a silly question, but do we need an AtomicReference? Could we not achieve the same by saving initializeFutureMonitor to a local variable in line 394 (and also on line 382) before saving it to the class field, and then mark the monitor as done using the local variable?
Is there a benefit of AtomicReference over the local variable?
There was a problem hiding this comment.
As far as I see it, at a minimum the initializeFutureMonitor needs to be made volatile because it is accessed from two different threads and the Job in createInitializeLanguageServerJob otherwise may never see the assigned value. The nice part of AtomicReference is that it offers compareAndSet() which can avoid another type of race conditions when the wrapper is stopped and restarted in quick succession. Also I find working with AtomicReference is more expressive regarding the intention than dealing with a volatile field.
When running the test suite on Windows and in Docker with Ubuntu, a lot of NPEs with the following stacktrace are thrown:
and because of the NPE random tests seem to fail.