-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PlainJarExportTests test fails due the monitor use in wrong thread #3718
PlainJarExportTests test fails due the monitor use in wrong thread #3718
Comments
At the first glance I have no idea how jdt.core could use ui tools like Display.asyncExec to delegate to ui thread. Anyone? @mickaelistria? At worst we would have to revert until solution found. |
|
ProgressMonitor can only be called in SWT Thread. partial revert of 20b7abd Keeping the CompilationProgress.isCanceled() eclipse-jdt#3718
The monitor passed to |
The change in compiler that reports to this "wrong" UI monitor now "uncovered" that problem of the wrong monitor being used in the build context. I believe reverting original change would be safest for M3, because monitor issues are known to popping up at most unexpected places... |
Ok, thanks. Did you manage to get a hint of where is this monitor created? I think it's a safe solution to wrap the updates to the monitor in some asyncExec when not in the UI thread. |
That ProgressMonitor is coming from way up in platform Buildmanager |
The monitor is from UI ModalContext, see stack:
|
Unfortunately, this change is breaking proper reporting when using a 3rd party compiler, so we have to re-add a workaround for it in the javac integration code. |
Since monitor methods may be invoked from any thread, make sure we wrap all SWT operations in UI Thread. Fixes eclipse-jdt/eclipse.jdt.core#3718
This should be a better fix: eclipse-platform/eclipse.platform.ui#2792 |
The question for me is if that would break on another use case (not covered by automated tests) where monitor is passed from UI to compiler => this would be too risky for M3 IMO. |
If only StatusLine has been correct a day before, this patch wouldn't have brought any concern although it would have been equally risky... |
That's life. Now we know that it is risky. |
Regression from #3711
3 tests fail now on all platforms, see for exampe https://download.eclipse.org/eclipse/downloads/drops4/I20250210-1800/testresults/html/org.eclipse.jdt.ui.tests_ep435I-unit-linux-x86_64-java21_linux.gtk.x86_64_21.html
Most relevant stack:
Full fail:
The text was updated successfully, but these errors were encountered: