Skip to content
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

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

Open
jjyr opened this issue Jan 6, 2018 · 7 comments
Open

Comments

@jjyr
Copy link

jjyr commented Jan 6, 2018

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.

@tarcieri
Copy link
Contributor

tarcieri commented Jan 6, 2018

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.

@jjyr
Copy link
Author

jjyr commented Jan 7, 2018

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.

@tarcieri
Copy link
Contributor

tarcieri commented Jan 7, 2018

@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.

@ioquatix
Copy link
Member

ioquatix commented Jan 9, 2019

@jjyr can you please submit a spec/PR.

@jjyr
Copy link
Author

jjyr commented Jan 16, 2019

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.

@tarcieri
Copy link
Contributor

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.

@ioquatix
Copy link
Member

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

Sounds perfect to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants