-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Mutexes should use .lock instead of .lockInterruptibly #4261
Comments
To be clear, this caused a nasty bug for Logstash that took a while to show up. |
I was about to comment that if we make this change, Thread blocked on Mutex acquisition would no longer be interruptible as in MRI:
However, JRuby does not appear to be actually interrupt the mutex correctly, and never reaches "after". |
@headius I suppose the confusion is that I'm not using I'm not sure if there's a way to disentangle those things, but I do think that'd make sense. That said, we learned our lesson and used At the very least I think it'd be desirable for the situation where there was no ruby |
It's difficult (maybe impossible) to know whether the interrupt is from someone in normal Java space doing @andrewvc: To clarify: you had a thread waiting on a mutex that was interrupted by Java code? I'd like to understand your bug better so we can figure out the best path forward. Meanwhile, I will push the |
@headius The gist of the issue is that the grok filter in logstash now monitors grokking threads for timeouts with a Here was the actual bug we hit, + the fix (moving to direct use of a |
Well it looks like CI is ok. So the remaining question for me (cc @enebo) is whether we should be looking to fix the uninterruptibility. As far as I can tell, MRI will not break out of a blocked I think we'll go with the patch you suggest for now and deal with any fallout. |
FWIW, here's what I get from MRI if I change that lock-interrupting script to try to
|
Merged to master in 6a436e9. |
Main thought that comes to mind would be going back to a hand-written |
Fixes jruby#5476. This reverts the original change from jruby#4261 that disabled lock interrupts, and uses Task logic from RubyThread to handle the interrupting properly.
Since ruby does not have checked exceptions the behavior here: https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/ext/thread/Mutex.java#L91 is very surprising. If users really need this behavior they should use a
ReentrantLock
directly.The only question I have now is if its removal might be even more surprising to existing users. Thoughts?
The text was updated successfully, but these errors were encountered: