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

Allow registration with empty interest #640

Merged
merged 2 commits into from Sep 1, 2017
Merged

Conversation

@rom1v
Copy link
Contributor

@rom1v rom1v commented Aug 2, 2017

In order to temporarily disable event listening for a specific handle, it may be convenient to reregister it with an empty interest.

This avoids to temporarily deregister the handle before registering it again (which, by the way, is not supported on Windows).

Therefore, remove the check that prohibited registration with empty interest.

Note that ERROR and HUP may still be triggered in that case.

@rom1v rom1v force-pushed the rom1v:empty_interest branch from ecff68f to 84d1182 Aug 2, 2017
@carllerche
Copy link
Member

@carllerche carllerche commented Aug 2, 2017

I think that I would rather get deregister to work (as I think it probably should). Internally, it probably is just a register w/ no args... Is that something you could try?

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Aug 2, 2017

I think that I would rather get deregister to work (as I think it probably should).

I agree that deregister (and reregister) should be implemented for Windows.

But even in the case deregister were working as expected, I think that empty interest should be allowed, since they are supported by the backends. For what it is worth, other APIs allow it (e.g. Java NIO).

In my client code using mio, for convenience I implemented a Poll wrapper that exposes an API accepting an empty Ready (and call register, deregister, reregister as necessary), which is disappointing since the low-level backends accept empty interest.

@rom1v rom1v force-pushed the rom1v:empty_interest branch from 84d1182 to b76e8c4 Aug 6, 2017
@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Aug 8, 2017

@carllerche On IRC, you were wondering if using empty interest was portable across all platforms.

So we need to know if it works for the backends supported by mio: epoll, kqueue, IOCP, and also the backend used fuchsia (I saw some commits recently about fuchsia).

man epoll suggests that epoll supports it:

The events member is a bit mask composed by ORing together zero or more of the following available event types

In order to test the "empty interest" behavior on each platform, I wrote a minimal sample that polls for readable events, then with empty interest, then for readable events again, so we can check that it behaves as expected: https://github.com/rom1v/mio-empty-interest

I executed it on Linux, Windows and Mac OS, I get the expected output:

interest: {readable}
0 -->
      --> 0
1 -->
      --> 1
interest: {}
2 -->
3 -->
4 -->
5 -->
6 -->
interest: {readable}
      --> 23456
7 -->
      --> 7
8 -->
      --> 8

I didn't test on fuchsia (@cramertj – or someone else –, could I ask you to execute this sample on fuchsia, please?).

The sample is maybe too minimal. Can you think of any specific situations were empty interest may lead to incorrect behavior?

I also use this patch in a "real-world" – not a sample – application where I often switch from/to non-empty/empty interests, it behaves as expected on Linux and Windows (I will test on Mac OS soon).

@carllerche
Copy link
Member

@carllerche carllerche commented Aug 12, 2017

So, what does it mean to 'register' w/ no interest? I could see reregister w/ no interest, but register is only supposed to be called when there is no interest... is a register w/ no interest basically a no-op?

rom1v added a commit to Genymobile/gnirehtet that referenced this pull request Aug 18, 2017
Currently, the mio library does not accept registering a handle with
empty interest. As a workaround, we provided a wrapper that exposed an
API that did, calling deregister/reregister internally as necessary.

However, this workaround does not work on Windows, because
deregister/reregister is not implemented:
<tokio-rs/mio#633>

Therefore, make mio accept empty interest directly:
<tokio-rs/mio#640>

This is implemented in our own fork, unless this change is accepted
upstream or we manage to find a solution that works with the upstream
version.
@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Aug 18, 2017

Concretely, it means calling epoll_ctl with op EPOLL_CTL_ADD and a struct epoll_event having a events field bitmask with EPOLLIN and EPOLLOUT unset.

This is convenient when you initialize your interests conditionally, then call the same code whatever its value (without taking care of deregistering/reregistering):

  • may I read? → if yes, enable readable
  • may I write? → if yes, enable writable
  • update interests
@carllerche
Copy link
Member

@carllerche carllerche commented Aug 18, 2017

You have a thought @alexcrichton ?

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Aug 18, 2017

If epoll itself implements this it seems fine to me at least!

@rom1v rom1v force-pushed the rom1v:empty_interest branch from b76e8c4 to 1514772 Aug 23, 2017
@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Aug 23, 2017

rebased to solve conflicts with master

Copy link
Member

@carllerche carllerche left a comment

Thanks for the PR & sorry for the delay.

I did a quick skim of the various platform docs and I think this should be an OK change. I have added a request to expand on a test, but the change seems good besides that.


assert!(poll.reregister(&sock, Token(0), Ready::empty(), PollOpt::edge()).is_err());
poll.reregister(&sock, Token(0), Ready::empty(), PollOpt::edge()).unwrap();

This comment has been minimized.

@carllerche

carllerche Aug 31, 2017
Member

Would you mind expanding the test to show that when a socket is received, no readable event is generated then once the listener is reregistered w/ readable interest, the readable event is generated (representing a pending accept).

This comment has been minimized.

@rom1v

rom1v Aug 31, 2017
Author Contributor

Yes, of course. Done :)

@carllerche
Copy link
Member

@carllerche carllerche commented Aug 31, 2017

Alright, PR looks good, now it's just a question of getting CI to pass.

@carllerche
Copy link
Member

@carllerche carllerche commented Aug 31, 2017

It seems like windows doesn't like the change :-/ I'm not sure exactly why yet. This may be a bigger issue w/ the custom event queue not being happy about registering w/ no interest.

@rom1v rom1v force-pushed the rom1v:empty_interest branch from 11f55be to 2029472 Aug 31, 2017
@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Aug 31, 2017

The registration of the client socket was missing, which is a problem on Windows.

The connect is not guaranteed to have started until it is registered
https://docs.rs/mio/0.6.10/mio/struct.Poll.html#registering-handles

I amended the commit to fix the issue.

@rom1v rom1v force-pushed the rom1v:empty_interest branch from 2029472 to 0089cde Aug 31, 2017
In order to temporarily disable event listening for a specific handle,
it may be convenient to reregister it with an empty interest.

This avoids to temporarily deregister the handle before registering it
again (which, by the way, is not supported on Windows[1]).

Therefore, remove the check that prohibited registration with empty
interest.

Note that ERROR and HUP may still be triggered in that case.

[1]: #633
@rom1v rom1v force-pushed the rom1v:empty_interest branch 2 times, most recently from 8283642 to 38d3710 Aug 31, 2017
Assert that poll() does not generate a readable event when registered
with empty interest, but that it generates the pending readable event as
soon as we reregister with a readable interest.
@rom1v rom1v force-pushed the rom1v:empty_interest branch from 38d3710 to f26e792 Sep 1, 2017
@carllerche carllerche merged commit c19881b into tokio-rs:master Sep 1, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rom1v added a commit to Genymobile/gnirehtet that referenced this pull request Sep 21, 2017
Use the upstream version of mio since this PR has been merged:
<tokio-rs/mio#640>
rom1v added a commit to Genymobile/gnirehtet that referenced this pull request Oct 30, 2017
Rust mio 0.6.11, the first release including the patch to support "empty
interest", is available.

Therefore, remove the temporary '[replace]' section in Cargo.toml, and
use the last 0.6.x version.

Also update other dependencies (generated by "cargo update").

<tokio-rs/mio#640>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.