-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
use synchronized block to avoid IllegalMonitorStateException #3526
Conversation
oh mannnn, all the CI actions decided to go nuts right on my PR |
what exactly does |
To test the changes in this pull request, install this apk: |
I was wondering exactly the same, that is why I was a bit skeptical when read that it is the solution to the problem in https://stackoverflow.com/a/24186147 but then I searched our code and in all other places where we use |
try { | ||
// The `wait()` needs to be enclosed in a while loop because there may be | ||
// "spurious wake-ups", i.e. `wait()` may return even though `notifyAll()` wasn't called. | ||
STOP_NOTIFIER.wait(); | ||
synchronized (STOP_NOTIFIER) { | ||
STOP_NOTIFIER.wait(); | ||
} | ||
} catch (InterruptedException ex) { } | ||
} | ||
} |
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.
According to the documentation, you're supposed to put the synchronized
around the while
?
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.
Since I can't create a suggestion here on GitHub (since it would touch unchanged lines), I just pushed a commit with my suggestion
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's amazing how hard these Java APIs are to be used correctly XD
Thanks for caring!
uses: actions/upload-artifact@v3 | ||
uses: actions/upload-artifact@v4 |
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.
Commited accidentally?
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.
CI fails otherwise (v3 is deprecated)
close #3525