nio4r icon indicating copy to clipboard operation
nio4r copied to clipboard

Pure ruby backend had very different behaviour to maintain Monitor#readiness

Open jjyr opened this issue 8 years ago • 7 comments

Problem

select and poll backend didn't clear monitor#readiness in Selector#select, but ruby backend did, it set each monitors readiness to nil in Selector#select. https://github.com/socketry/nio4r/blob/master/lib/nio/selector.rb#L87

Reproduce

Run this script to start a modified example/echo_server, then run telnet localhost 1234 several times. can see different behaviour

use backend ruby
Listening on localhost:1234
server readiness is 
*** ::1:52414 connected
server readiness is r
*** ::1:52414 disconnected
server readiness is 
use backend select
Listening on localhost:1234
server readiness is w
*** ::1:52423 connected
server readiness is r
*** ::1:52423 disconnected
server readiness is r
*** ::1:52424 connected
server readiness is r
*** ::1:52424 disconnected
server readiness is r

Affect

In my case, I save each monitor objects, and use monitor#readable? to detect it status after invokeSelector#select (this method)

Maybe it's not right way to use monitor, but nio4r should maintain the right behaviour and document it.

jjyr avatar Jan 06 '18 09:01 jjyr

Just to confirm I understand the problems:

  1. you are checking readiness on a monitor that was not selected by Selector#select and getting a stale value
  2. the behavior is inconsistent between the pure Ruby and libev backends

One option I can think of is to keep a counter inside of NIO::Selector of the number of times #select has been invoked, and when monitors are selected setting the same counter value on the monitor itself. If you invoke #readiness on the monitor and the counts do not match, it could either return nil or raise an exception, e.g. ReadinessError: monitor has not been selected for any registered interests

I kind of like the latter, because it doesn't make sense to check the readiness of monitors that were not selected by the last NIO::Selector#select invocation, so doing so is effectively a bug in the caller's code.

tarcieri avatar Jan 06 '18 15:01 tarcieri

I'm also prefer latter one, to maintain a counter is not necessary, since readiness become unknown status between actually io operation and next Selector#select, it seems not big help to caller.

Maybe should remove this line, to make pure ruby backend according to libev behaviour.

jjyr avatar Jan 07 '18 04:01 jjyr

@jjyr the reason for the counter is it lets us tell if monitors were selected by the most recent select operation without having to modify any state in the unselected monitors.

We could remove that line in selector.rb, but the result would be both backends consistently giving you stale results. I'm not sure exposing stale results is a good idea.

tarcieri avatar Jan 07 '18 19:01 tarcieri

@jjyr can you please submit a spec/PR.

ioquatix avatar Jan 09 '19 07:01 ioquatix

Now I think we should not maintain consistency for a wrong use case, we either need a better design to avoid misuse or we just leave it here.

jjyr avatar Jan 16 '19 15:01 jjyr

The counter approach seems simple enough, otherwise I think we can simply document that the readiness state of unselected monitors is undefined behavior that may vary between backends.

tarcieri avatar Jan 16 '19 19:01 tarcieri

we can simply document that the readiness state of unselected monitors is undefined behavior that may vary between backends.

Sounds perfect to me.

ioquatix avatar Jan 16 '19 19:01 ioquatix