ArgumentError: this IO is already registered with selector #121
Comments
|
The function that causes the failure is here: Not sure how that could possibly be failing.. perhaps the ensure block? Some kind of weird flow control due to a timeout somewhere? |
|
Okay, yeah, might be exactly the same issue as #101, because Hmmmmmm... |
|
Ah, damn it, it's 5:30AM already. |
|
I believe there is a bug in I made a test case in RubyDNS which mostly hits the error condition systematically: https://github.com/ioquatix/rubydns/blob/master/spec/rubydns/celluloid_bug_spec.rb I augmented celluloid-io with some debugging output: # Wait for the given IO operation to complete
def wait(io, set)
# zomg ugly type conversion :(
unless io.is_a?(::IO) or io.is_a?(OpenSSL::SSL::SSLSocket)
if io.respond_to? :to_io
io = io.to_io
elsif ::IO.respond_to? :try_convert
io = ::IO.try_convert(io)
end
raise TypeError, "can't convert #{io.class} into IO" unless io.is_a?(::IO)
end
puts "-> wait(#{io} -> #{io.inspect}, #{set})"
monitor = @selector.register(io, set)
monitor.value = Task.current
Task.suspend :iowait
puts "<- wait(#{io}, #{set})"
endI checked several times and I had the argument error but never noticed entering I added I'm going to assume that somehow, the Perhaps you can give me some feedback on what is going on here. |
|
@tarcieri Can you please check this? |
|
@ioquatix oh, I see what might be happening. Yeah, you're right, except: https://github.com/celluloid/celluloid-io/blob/master/lib/celluloid/io/reactor.rb#L51
We should probably move |
|
Okay, do you want me to submit a PR? I'm pretty keen to get this sorted as it's causing bugs in the latest RubyDNS release. |
|
@ioquatix sure! Delete the |
|
I tried adding it to an ensure block like you said but Ruby segfaults. Removing the ensure block, but just leaving it at the end (i.e. |
|
Sorry to bump this (don't mean to be annoying), but have you made any progress on this? |
|
I haven't had a chance to take a look |
|
@tarcieri Did you get a chance to take a look? |
|
no |
|
bump :) |
|
No progress, sorry |
|
Bumped into it as well. It seems #101 is not as fixed as we thought... Here is a small gist: # ran with ruby 2.0.0
require 'celluloid/io'
class Dad
include Celluloid::IO
def initialize
@sock = TCPSocket.new("localhost", 22)
end
def wiriwiri
begin
timeout(2) do
loop do
@sock.wait_readable
end
end
rescue
@sock.wait_readable
end
end
end
Dad.new.wiriwiri I theory, this script shouldn't blow. I force the endless state of waiting on socket to be readable. When the timeout happens, the socket should be fred from the reactor. But this is not happening. My "theory": the timeout failed message doesn't get prioritized in the mailbox, or does not trigger the reactor select. And the next evaluated message uses the socket which is still under the reactor registered collection. One flaw IMHO from the celluloid io API when comparing to the native ruby implementation is that: IO::Select's last parameter is a timeout for waiting on descriptors. This is how most evented implementations handle timeouts: they IO::select for a certain time period. An example: https://github.com/net-ssh/net-ssh/blob/043bbca9b65cb7f8d862371f66cd4e163287291b/lib/net/ssh/transport/packet_stream.rb#L75 First example checks whether socket is available for reading by passing 0. Second example waits for a definite period on descriptors. So, this is the main thing about it: celluloid-io doesn't allow me to wait on a socket for a definite period of time. My proposal would be to pass an optional 3rd parameter to the #wait method: a timeout. when not passed (nil), it defaults to the current behaviour and waits indefinitely (like IO::select). when passed a parameter, it times out, deregisters the socket and return true/false, whether socket is available or not. Actually, @ioquatix and me have been using the timeout feature because this API is not available (correct me if I'm wrong). Let me know what you think. I think this would mean some changes on nio4r, probably. Anyway, the "recover from timeout" thing must succeed first. |
|
@ioquatix, I used the monitor.close call in the ensure block and it worked well for me. Can you troubleshoot exactly where does the segfault come from? My reasoning for it is: if an exception occurs while the fiber is paused, what happens to it? Does it get resumed? Can we say for sure that it will always safely close? |
|
@ioquatix , found one way to "force" the stackdump when the monitor.close is in the ensure block: when on timeout loop, you send a SIGTERM to the process it. Also found out what I wanted: if not inside the ensure block, the time out will not jump to the instructions after task suspending, hence it will not close the monitor and fail with "ArgumentError: this IO is already registered with selector". |
|
here's the stack dump (the relevant nio4r part): @tarcieri , if this might be helpful. |
|
Correction: "timers" seems to be the issue (not handling SIGTERM properly on some c extension??) |
...and the first thing anyone wants to do is wrap this up into an event loop that can calculate that timeout based on any number of potential timers.
Celluloid is a full-featured event system complete with timers, and expects you to use that functionality to implement timeout handling. Adding timeouts with Celluloid::IO (now that they actually work) is as simple as: timeout(n) { io_operation_here }Timeouts are handled by the reactor: https://github.com/celluloid/celluloid-io/blob/master/lib/celluloid/io/reactor.rb#L48 |
|
@tarcieri , not what I meant at all. I'm fully aware that the reactor handles the timeouts, I meant more of a way of "replacing" native calls with celluloid calls to select, like in the net-ssh example, which would "in theory" involve overwriting the ruby_compat module. This could be then applied to other "native" libraries that use the IO::select approach. But I don't want to move the attention away from the bug above :) Tomorrow I'll get back to this. |
|
Hi Guys, I suggest we have a group Google Hangout to bump heads on this issue and see if we can resolve it. Are you interested? |
|
@TiagoCardoso1983 can you please confirm you were using Timers 4.0.1 for your example above? |
|
@TiagoCardoso1983 I tend to agree with @tarcieri on the timer issue, explicit timeouts are a real pain especially with nested APIs. In RubyDNS, this is used in the resolver: https://github.com/ioquatix/rubydns/blob/master/lib/rubydns/resolver.rb#L101-L103 In this example, it would be a pain to track the timeout through several read/write calls. |
|
@ioquatix , I mostly agree with both, I'm just getting frustrated with net-ssh (another topic, another already existing issue ;) ). But I've been working around it. Concerning this, yes, I confirm it was the timers 4.0.1 (and nio4r 1.0.1) It seems by the stack dump it is coming from either hitimes or the nio4r libev extensions. |
|
Forgot also to had this missing dump: |
seems also relevant, even though I must admit I have 0 experience with C extensions. |
|
The offender might then have been this line: https://github.com/celluloid/nio4r/blob/master/ext/libev/ev.c#L3659 @tarcieri , shouldn't the assert call "bubble up" to ruby? |
|
asserts get disabled when compiled with -DN(O?)DEBUG Von einem mobilen Gerät gesendet
|
|
@TiagoCardoso1983 my proposed changes fix the problem https://github.com/ioquatix/celluloid-io/commits/fix-monitor but I'm not convinced it is the correct solution. Can you please try it out and let me know what you think? I can run your example given above and it works. Correctly? Not sure about that, but it doesn't blow up. |
|
Specifically this commit: ioquatix@6eb2bd8 |
|
@ioquatix it does correct the segfault. I do get an IOError: selector is closed, which is probably ok (I don't see where the socket gets closed though). I'd say it's good to go! |
|
So the ensure block triggered the segfault? Does that mean that one of the C extensions isn't handling ensure properly? |
|
We could merge ioquatix@6eb2bd8 for now and investigate adding an ensure later |
|
My proposed solution using ensure is ioquatix@a6a8ad7 |
|
I think merging this but not releasing it might be a good first step. We can test it. I believe that we need to make some specific tests to hit this case so we don't have regressions in the future. We also want to make sure this is the correct logic. I just moved lines around to see if it would work based on @tarcieri's advice, but I don't actually know WHY it fixed the issue completely, or if it HAS fixed the issue.. |
|
Regardless of why it corrects the problem, it's still a better, easier-to-understand factoring, IMO |
|
Want to open a PR? |
|
Okay cap'in, I'll open a PArrrrrgh. Yarrrr. |
Can't for the life of me figure out why this is happening, albeit very rarely:
e.g. https://travis-ci.org/ioquatix/rubydns/jobs/38832552
I believe we may have discussed this a bit here too: #101 but I'm raising a new issue since that stuff is all so old.
The text was updated successfully, but these errors were encountered: