-
Notifications
You must be signed in to change notification settings - Fork 475
Fix usages of some async utilities #1610
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
base: master
Are you sure you want to change the base?
Conversation
212fc78 to
fb6775e
Compare
|
Hmmm, the tests failing now were previously flaky at best. They either were successful because the code behaved properly, E.g. for @leonard84 do you have any idea how to better properly test this, without depending on "high enough" timeouts? |
|
Maybe we should not really test the concurrency but trust that JUnit Platform does the right thing and instead just test that the annotations set the proper exclusive resources on the feature infos? |
| while (waitMillis > 0) { | ||
| long waitStart = System.nanoTime(); | ||
| try { | ||
| synced = sync.offer(stackTrace, waitMillis, TimeUnit.MILLISECONDS); | ||
| } catch (InterruptedException ignored) { | ||
| // this is our own thread, so we can ignore the interruption safely and continue the remaining waiting | ||
| waitMillis -= TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - waitStart); | ||
| continue; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it indeed happens that the watcher thread is interrupted during the initial wait, it should continue the initial wait, shouldn't it?
Or should it really forward the interrupting to the Feature thread, just because it got interrupted for whatever reason?
| return; | ||
| } | ||
| } catch (InterruptedException | ExecutionException e) { | ||
| ExecutionResult executionResult = iterationRunner.runIteration(args, maxIterations).join(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using join to make it un-interruptible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not get uninterruptible.
It just does not throw checked exceptions but unchecked ones.
If you use get() you might get InterruptedException and then have to handle it properly.
Just ignoring is not ok at that spot as this is not a self-managed thread, so in most cases you should either rethrow or at least re-interrupt so that code higher in the stack can properly handle the interrupt and so on.
And by using join() here you can simply get around all that, as you don't mess with the interrupted exception yourself.
And ExecutionException would never come here anyway, as the future is never completed exceptionally.
fb6775e to
1d4ecc0
Compare
Just increasing the timeout to 10s doesn't fix the tests, we'd actually need a way to get access to the lock. Some time ago, I advocated that the JUnit Platform integrates JFR events, to enable low tracing of parallel executions. Unfortunately, that didn't gain any traction with the sentiment being that the listener->jfr events was good enough. Maybe this would be a good showcase why it would be good to have those low-level events. If the platform would emit events for locking/unlocking of exclusive resources, we could make assertions upon those events instead of not knowing why if it was ok, because the code worked properly or because there was just a single thread for some reason. @marcphilipp WDYT? |
1d4ecc0 to
7ba0d6f
Compare
I'm not opposed to it in general. The main concern was breaking users' builds. Apparently there are still JDKs that don't include the JFR module (see junit-team/junit-framework#3279 (comment)) but we might now be able to use jfr-polyfill. |
f337286 to
776db33
Compare
776db33 to
5b9d4d6
Compare
5b9d4d6 to
8f11cc6
Compare
|
Any news on that @marcphilipp? |
cd2a9e1 to
3764821
Compare
c4dcae4 to
1b4069e
Compare
eced08a to
012ca2c
Compare
a536962 to
4a09c7a
Compare
73fabaa to
d9e0b17
Compare
afd35f8 to
1f26a18
Compare
1f26a18 to
452e079
Compare
5da14be to
4d7c7f3
Compare
073b436 to
3555bc4
Compare
0a3bb6f to
c0490ef
Compare
1d07b04 to
2010fb7
Compare
b71eade to
7796b67
Compare
7796b67 to
8abf460
Compare
7753e5c to
7c6a8a6
Compare
7c6a8a6 to
a95734e
Compare
a95734e to
02f31fb
Compare
02f31fb to
3afb8a7
Compare
Fixes #1600