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

Closed
HoneyryderChuck opened this Issue Jul 21, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@HoneyryderChuck

HoneyryderChuck commented Jul 21, 2015

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.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 21, 2015

Member

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.

Member

headius commented Jul 21, 2015

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.

@HoneyryderChuck

This comment has been minimized.

Show comment
Hide comment
@HoneyryderChuck

HoneyryderChuck Jul 21, 2015

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:

[1] pry(main)> require 'timeout/extensions'                            
=> true                                                                
[2] pry(main)> method(:timeout).source_location                        
=> ["/home/timeout-extensions/lib/timeout/extensions.rb", 16] 
[3] pry(main)>                                                         

But then, in a script, doing something inside TCPSocket class from Celluloid (using debugger):

irb(#<Celluloid::IO::TCPSocket:0x13dac87a>):002:0> method(:timeout).source_location     
=> nil

this call to timeout jumps to the jruby implementation, according to the backtrace I receive:

        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:35:in `block in available_for_read?'           
        org/jruby/ext/timeout/Timeout.java:123:in `timeout'                                                      
        org/jruby/ext/timeout/Timeout.java:92:in `timeout'                                                       
        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:34:in `available_for_read?'                    
        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:47:in `next_packet'                            

(the available_for_read? method calls the timeout method, should have jumped to the timeout-extensions method).

Maybe has to do with alias_method?

HoneyryderChuck commented Jul 21, 2015

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:

[1] pry(main)> require 'timeout/extensions'                            
=> true                                                                
[2] pry(main)> method(:timeout).source_location                        
=> ["/home/timeout-extensions/lib/timeout/extensions.rb", 16] 
[3] pry(main)>                                                         

But then, in a script, doing something inside TCPSocket class from Celluloid (using debugger):

irb(#<Celluloid::IO::TCPSocket:0x13dac87a>):002:0> method(:timeout).source_location     
=> nil

this call to timeout jumps to the jruby implementation, according to the backtrace I receive:

        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:35:in `block in available_for_read?'           
        org/jruby/ext/timeout/Timeout.java:123:in `timeout'                                                      
        org/jruby/ext/timeout/Timeout.java:92:in `timeout'                                                       
        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:34:in `available_for_read?'                    
        /home/sem-celluloid/lib/sem/celluloid/net/ssh.rb:47:in `next_packet'                            

(the available_for_read? method calls the timeout method, should have jumped to the timeout-extensions method).

Maybe has to do with alias_method?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 21, 2015

Member

I figured the issue out. In MRI, where timeout is pure Ruby, the top-level timeout method is defined like this:

# 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!

Member

headius commented Jul 21, 2015

I figured the issue out. In MRI, where timeout is pure Ruby, the top-level timeout method is defined like this:

# 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!

@HoneyryderChuck

This comment has been minimized.

Show comment
Hide comment
@HoneyryderChuck

HoneyryderChuck Jul 22, 2015

No mention, glad to help 👍

HoneyryderChuck commented Jul 22, 2015

No mention, glad to help 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment