-
-
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
timeout method not calling Timeout::timeout #3158
Comments
Interesting. I would have expected that these methods were implemented like other module methods, with two copies of the method in two method tables, but I suppose in MRI the instance method just dispatches to the class method. That's not a difficult change to make. I expect this affects 1.7 too. |
Humm... it's interesting, according to this, you are right, it should. I tried patching the Kernel.timeout, even: extend Timeout::Extensions and, if I see load it in the console, it's overwritten:
But then, in a script, doing something inside TCPSocket class from Celluloid (using debugger):
this call to timeout jumps to the jruby implementation, according to the backtrace I receive:
(the available_for_read? method calls the timeout method, should have jumped to the timeout-extensions method). Maybe has to do with alias_method? |
I figured the issue out. In MRI, where timeout is pure Ruby, the top-level # Identical to:
#
# Timeout::timeout(n, e, &block).
#
# This method is deprecated and provided only for backwards compatibility.
# You should use Timeout#timeout instead.
def timeout(n, e = nil, &block)
Timeout::timeout(n, e, &block)
end Note the deprecation. In JRuby, it's defined as a direct static call in Java to the actual implementation. The Timeout::timeout method is defined as a normal module method in both MRI and JRuby, though, so that part's ok. I'll have a fix for this momentarily. Thanks for the report! |
No mention, glad to help 👍 |
I'm supporting and using a library which overwrites the timeout method:
https://github.com/celluloid/timeout-extensions
It's kind of a "monkey-patch-fu" extension for celluloid, as celluloid has its own timeout method and currently ruby doesn't support an API for switching timeout methods depending of method.
Currently only the Timeout module is being patched, here
This was done just because
But in JRuby (9.0.0.0.rc2) it seems that timeout and Timeout::timeout are not the same. Calling timeout jumps straight to the Java implementation, which breaks this patch. So, instead of one, I have to patch two methods for JRuby. Should JRuby follow the standard from MRI in this case, or is there a reason? I would prefer not to patch the same thing two times.
The text was updated successfully, but these errors were encountered: