Use timer unconditionally in BusyIndicator.showWhile(Future)#3167
Use timer unconditionally in BusyIndicator.showWhile(Future)#3167akurtakov wants to merge 1 commit intoeclipse-platform:masterfrom
Conversation
There was a problem hiding this comment.
Thank you for proposing a solution for the BusyIndicator getting stuck. Having that fixed so that CI executions terminate regularly again will significantly improve the contributor experience ,
This appears to be a reasonable solution for avoiding that the BusyIndicator gets stuck. It does not add any considerable overhead but makes the implementation more robust against the wake() call once the Future has finished not having the intended effect.
I see two possible improvements but consider neither of them mandatory for further processing the PR:
- After existing the display spinning loop (because the future is done and the display was woken up) the timerExec might be cancelled to avoid an unnecessary wakeup of the display (but it should not be a problem to have that unnecessary wakup though).
- Using this timer even with a
CompletionStagefuture might be limited to platforms on which we know about problems withwake()calls (i.e., in particular Linux). But as said above, the overhead is so minimal that it may be considered irrelevant, so making the behavior platform-specific may introduce unnecessary complexity.
This change might also be reverted once #3060 is addressed.
And there is small chance that the underlying issue is addressed by #3161 (as also indicated in #3161 (review)), so we should wait for some builds with that fix and see if the BusyIndicator test still gets stuck.
|
Turns out that #3161 did not fix the issue of the BusyIndicator test getting stuck. #3166 was just rebased on master including the the mentioned change, but the current test execution got stuck at the BusyIndicator test again: |
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
I don't think this is the right direction.
It changes production code that never was reported to be stuck instead of fixing the problem (we just do guess working here unless someone was able to reliable reproduce it) and hiding a possibly serve bug in SWT.
I would be fine if we maybe enhance the test with an additional guard e.g. lets say we start a (java) timer that hwne it fires constantly tries to wake up the SWT thread and make the test then fail an assertion.
|
@HeikoKlare if the problem is currently visible then I think the next easy fix would be to try out making it volatile: |
It's based on your own assessment that the issue might only be visible on test execution but not in productive usage because there are other events happening because of human input that wake up the display: #3044 (comment)
Yes, we could do that. But then we should proceed with that soon. We could also just revert this change once #3060 is addressed and proves to fix the issue. I just don't think we should further wait for other options to maybe fix the issue if no one is working on them while we have a proposed solution that seems to work and does not harm. The CI builds freqeuently getting stuck are quite annoying and limit productivity, so have a timely solution (even if it's a temporary one) would be great. |
|
I fully agree that current state can not continue . I would say - if we don't have another fix in and tests still stuck on Monday, let's merge this one as it's already 2 months with this state. |
I disagree here: The change is not backed by any proof it will actually fix the stucked test, this is just guess working, I see not any trace that this is guaranteed to work and it changes code I use every day and never was hanging since it was introduced. In the worst scenario I would better disable the test than changing the implementation for strange CI behavior we see here. I already suggested a workaround that would only change test code and gave more insights... I now prepare a PR with my proposal and then we can see. |
Should prevent hangs like eclipse-platform#3044
|
There have been 2 full rebuilds of this PR without any of the verification jobs stuck. I've just rebased it for third run - if nothing is stuck again , this is a proof enough for me as there haven't been any other PR lately that had even 2 good builds in a row. |
|
CI builds get stuck so often because of the issue that we will easily see if this fix has an impact or not once it has been merged and can, if necessary, simply revert it. Anyway, I think we have a good proposal/agreement now: let's have a look at the next proposals, such as the upcoming one from Christoph, and see if we can make progress with that. If we do not find a more suitable solution until Monday, let's proceed with this one. |
I have builds with 20 reruns and without an issue, so that's not really a proof. Anyways I still think it is not the right think to fix a test failure only ever reproducible on CI and only ever through certain times. |
|
Analysis have shown better ways to do in the original issue. |
3 participants
Should prevent hangs like
#3044