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

Poll.deregister() has no effect on Windows #633

Closed
rom1v opened this issue Jul 18, 2017 · 12 comments
Closed

Poll.deregister() has no effect on Windows #633

rom1v opened this issue Jul 18, 2017 · 12 comments
Projects
Milestone

Comments

@rom1v
Copy link
Contributor

@rom1v rom1v commented Jul 18, 2017

In an application, I sometimes need to deregister a socket, and register it later. In that case, the second register() fails on Windows.

I reproduced the problem in the following sample:

extern crate mio;
use std::net::{Ipv4Addr, SocketAddr};
use mio::{Ready, Poll, PollOpt, Token};

fn main() {
    let addr = SocketAddr::new(Ipv4Addr::new(127, 0, 0, 1).into(), 1234);
    let stream = mio::tcp::TcpStream::connect(&addr).unwrap();

    let poll = Poll::new().unwrap();

    poll.register(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();
    poll.deregister(&stream).unwrap();
    poll.register(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();

    println!("It works!");
}

It works on Linux, but fails on Windows:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 87, message: "The parameter is incorrect." } }', src\libcore\result.rs:859

Interestingly, if we replace the last call to register() by reregister(), then it fails on Linux (as expected), but it works on Windows:

    poll.register(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();
    poll.deregister(&stream).unwrap();
    poll.reregister(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();

In fact, on Windows, it behaves exactly as if we didn't call deregister() at all:

    poll.register(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();
    // poll.deregister(&stream).unwrap(); // ignored on Windows
    poll.register(&stream, Token(0), Ready::readable(), PollOpt::level()).unwrap();
@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Jul 18, 2017

Yes htis is expected on Windows where deregister isn't supported on Windows, but it's supported on Linux.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Jul 18, 2017

Hmm, so we cannot deregister? What's the expected usage on Windows, then? (we can't register new handles indefinitely without deregistering them)

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Jul 18, 2017

On Windows once you associate a handle with an IOCP object you can't undo that (AFAIK), so whenever you register an object it's permanently associated with that event loop.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Jul 18, 2017

OK, thanks for the explanation. 👍

I don't know Windows API, but I found this answer: https://stackoverflow.com/a/35373536/1987178

What do you think?

IMO we need a thin wrapper that exposes deregister() on Windows in some way.

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Jul 18, 2017

Sounds like a possibility yeah!

rom1v added a commit to rom1v/mio that referenced this issue 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[1]).

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

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

[1]: tokio-rs#633
rom1v added a commit to rom1v/mio that referenced this issue 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[1]).

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

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

[1]: tokio-rs#633
@In-line
Copy link

@In-line In-line commented Aug 4, 2017

Maybe we can use select call?

rom1v added a commit to rom1v/mio that referenced this issue Aug 6, 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]: tokio-rs#633
@carllerche
Copy link
Member

@carllerche carllerche commented Aug 12, 2017

@In-line we cannot use select on windows

rom1v added a commit to Genymobile/gnirehtet that referenced this issue 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 added a commit to rom1v/mio that referenced this issue Aug 23, 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]: tokio-rs#633
rom1v added a commit to rom1v/mio that referenced this issue 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]: tokio-rs#633
rom1v added a commit to rom1v/mio that referenced this issue 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]: tokio-rs#633
carllerche added a commit that referenced this issue Sep 1, 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
@hawkw hawkw added this to Needs triage in Triage Aug 17, 2018
@carllerche
Copy link
Member

@carllerche carllerche commented Jan 13, 2019

If this is still an issue, it probably can be fixed by tracking the necessary state on the windows side of things.

@SergejJurecko
Copy link

@SergejJurecko SergejJurecko commented Jul 15, 2019

Yes it still an issue.

This project has it implemented and it is a pretty simple solution: https://github.com/mitsuhiko/rust-listenfd/blob/89a37b36b749536b9cc079be2e1ac25f36b74b52/src/windows.rs#L56

I've tried and putting this in windows deregister call works and registering socket again later also works. I can create a PR if it is desired to implement it this way?

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw commented Jul 15, 2019

@SergejJurecko a Windows rewrite is in the works (#1034), any chance you can try that?

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw commented Jul 20, 2019

In Mio v0.7 registering after deregister is invalid, however reregistering after deregistering is valid for which I added a test in #1043. Incorrect see next comment.

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw commented Jul 20, 2019

I was incorrect in my previous comment, in Mio v0.7 reregistering after deregister is invalid, however registering after deregistering is valid.

Triage automation moved this from Needs triage to Closed Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
  
Closed
Linked pull requests

Successfully merging a pull request may close this issue.

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