-
-
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
Anonymous class-type being raised by Timeout/Queue in both 9x and 1x. #3834
Comments
Partial fix for #3834 The main issue here is that we frequently return this for DynamicMethod#dup, rather than return a new instance, and then this gets modified by code expecting it to be something different. This breaks reflective code because many reflective methods will expect the methods in a module's method table to say their implementationClass is that module. The case in #3834 broke because the accessor methods in Struct have this "fake" dup logic, so when the struct class itself was duplicated, the methods got modified implementationClass = clone and seemed to "disappear" from the original. This is only a partial fix because it just fixes those accessors in Struct. The proper fix would be to make sure DynamicMethod#dup returns a real new object for all DynamicMethod (except "dead" ones like UndefinedMethod). This may also fix (or make easier to fix) issues like #2988, where a dup'ed module's methods do not reflect their new implementationClass. Specs pending.
@headius here's the full backtrace as requested: |
@digitalextremist Ok, yeah...the exception seems to be raised by someone else via our thread eventing. That's harder to investigate since we don't have logging of cross-thread exception raises right now. The exception itself looks like it's probably the anonymous exception Timeout throws. It's unusual to me that it's bubbling out of a yield. Timing out in yield would mean that fiber A resumes fiber B, fiber B yields back to A, and A times out and bubbles this exception back to B. That's an unusual direction for exceptions to flow...generally they only come out of |
Ok, in order to continue on this I think we need a reproduction, or @digitalextremist will need to do some debugging locally. A patch to start investigating this would be to add logging to ThreadFiber.exchangeWithFiber's catch block to print out all exceptions. I suspect it is propagating something the wrong direction. |
I remember there used to be a Thread-based mode for Celluloid too...perhaps it still exists and we can try it? |
Another place to debug would be |
@headius I was able to work around this for now by cutting out Either way I remember the situation clearly and will try to come back with a repro that works. I think the key here in reproducing it reliably would be to have a Thank you very much for looking through this. Our interactions caused me to think of the workaround, brought up things in |
Still a bit of a mystery and unlikely to be investigated for the quick 9.1.1 release. Punting. |
@headius this resurfaced with In that incarnation, it appears to be Which makes sense, because here is what complains: |
Perhaps I missed it, but what was the actual exception class that bubbled out? |
Pardon me, it's |
Ok...so is this just getting through because nobody expects to rescue it? Is there anything we need to fix on the JRuby side? I assume since the same issue appeared on Rubinius, it may be an exception-handling gap in Celluloid... |
I'm troubleshooting the |
Ok, I can at least investigate why the class is anonymous. |
It looks like the anonymous exception code is very old...no longer present in MRI's timeout.rb after August 2013. I will do a pass over timeout.rb and our version and try to ensure they match. |
Ok, so here's the deal. If you provide a specific exception type to At one point, it did Class.new(Exception) every time you called for a timeout guard. This is the anonymous exception we emulate currently (inside an exception type called AnonymousException but with its name removed to match MRI. Later, they realized the high cost of creating a new class every time and instead created a single ExitException type they used to unroll. And most recently it appears they now simply raise Timeout::Error right away but post-process it a bit to scrub out the backtrace lines related to The original intent was that this should be an exception users never see and never rescue so it propagates all the way out to the timeout call. Now, the fact that you saw this exception makes me think we're not properly handling it when it bubbles out, and it's continuing to propagate into user code. That could be a bug, or more likely it's a problem with the asynchronous nature of Timeout causing the anon exception to be raised outside the timeout call. I will do some experimenting and see if I can get this to reproduce based on my theory. |
Our native Timeout has not been updated in a long time, and for * A separate anonymous exception is not used to trigger the timeout anymore; instead, they use catch/throw. * The exception message string is pre-allocated and frozen. * Various other miscellaneous changes. This version has *not* been tested for performance, and it will initially be much slower than our scheduled executor-based version due to it always spinning up a timeout thread. But we can see how it goes in test suites and then evaluate how to move forward. Note that the change to test/jruby/test_timeout.rb was to bring behavior in line with MRI 2.3.
I have pushed a I doubt will stick with unmodified |
Our native Timeout has not been updated in a long time, and for * A separate anonymous exception is not used to trigger the timeout anymore; instead, they use catch/throw. * The exception message string is pre-allocated and frozen. * Various other miscellaneous changes. This version has *not* been tested for performance, and it will initially be much slower than our scheduled executor-based version due to it always spinning up a timeout thread. But we can see how it goes in test suites and then evaluate how to move forward. Note that the change to test/jruby/test_timeout.rb was to bring behavior in line with MRI 2.3.
Our native Timeout has not been updated in a long time, and for * A separate anonymous exception is not used to trigger the timeout anymore; instead, they use catch/throw. * The exception message string is pre-allocated and frozen. * Various other miscellaneous changes. This version has *not* been tested for performance, and it will initially be much slower than our scheduled executor-based version due to it always spinning up a timeout thread. But we can see how it goes in test suites and then evaluate how to move forward. Note that the change to test/jruby/test_timeout.rb was to bring behavior in line with MRI 2.3.
Our native Timeout has not been updated in a long time, and for * A separate anonymous exception is not used to trigger the timeout anymore; instead, they use catch/throw. * The exception message string is pre-allocated and frozen. * Various other miscellaneous changes. This version has *not* been tested for performance, and it will initially be much slower than our scheduled executor-based version due to it always spinning up a timeout thread. But we can see how it goes in test suites and then evaluate how to move forward. Note that the change to test/jruby/test_timeout.rb was to bring behavior in line with MRI 2.3.
Workarounds include not using timeout and using it with an explicit exception, so I think we can punt any remaining rewrite of timeout.rb (or re-incorporation of MRI's version, with bottlenecks fixed) to 9.1.4.0. |
Fixes jruby#3834. * Use Timeout::Error instead of leaky anonymous exception. * Move executor to instance data from static. * Clean up redundant, outdated code. * Move sharable pieces back into timeout.rb. * Eliminate builtin and load ext from timeout.rb.
I've pushed an updated implementation of Timeout that shares more code with MRI and always uses the same Timeout::Error exception rather than our weird anonymous one. I believe this will resolve this issue. |
Linux 3.13.0-48-generic #80-Ubuntu SMP x86_64
Both...
jruby 1.7.22 (2.0.0p598) 2016-03-25 c28f492 on Java HotSpot(TM) 64-Bit Server VM 1.8.0_77-b03 +jit [linux-amd64]
jruby 9.0.5.0 (2.2.3) 2016-03-30 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [linux-amd64]
Working with
Celluloid
, and thehttp.rb
gem, usingTimeout.timeout
onHTTP
calls; in specific cases I can always crash an actor because I am unable to catch the exception raised.I expected
Timeout.timeout
to raiseTimeout::Error
...not this:You can see by the message that it's a timeout, but the class which is raised bearing that message and backtrace is not
Timeout::Error
.I've tried everything to catch that class type. Including using this sort of
rescue
:... which ought to be redundant in the extreme, but the class still escapes capture.
Unfortunately, to post a reproducible piece of code I would need to extract some from client code, but also replicate conditions that I've isolated in a certain context:
This is no normal
HTTP
timeout, though it does happen in that case at times -- less reliably. But it always happens when we take all nodes out of a load balancer, and then hit the load balancer with aGET
request.The text was updated successfully, but these errors were encountered: