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

Expose net::driver::Watcher mechanism #293

Closed
dvc94ch opened this issue Oct 9, 2019 · 21 comments
Closed

Expose net::driver::Watcher mechanism #293

dvc94ch opened this issue Oct 9, 2019 · 21 comments
Labels
api design Open design questions

Comments

@dvc94ch
Copy link

dvc94ch commented Oct 9, 2019

Crates implementing Evented should be able to register a reactor.

Follow up from discussion #65

@yoshuawuyts yoshuawuyts added the api design Open design questions label Oct 9, 2019
@yoshuawuyts
Copy link
Contributor

This API still requires some work, but just to set expectations: it's unlikely we'll expose mio::Evented in the API bounds. Instead we're more likely to use AsRawFd for Unix and AsRawSocket for Windows for compat with std.

On top of that we should also provide a way to access the epoll / kqueue / iocp instance directly so people can perform syscalls on it outside of the read_with / write_with APIs.

@dvc94ch
Copy link
Author

dvc94ch commented Oct 13, 2019

I think this is really important. Just got blocked trying to set the ecn bits on a udp datagram. Most async system programs need this in some form.

@ghost
Copy link

ghost commented Nov 2, 2019

@dvc94ch What if we had an API along these lines? Would that work for you?

enum Interest {
    Read,
    Write,
    Both,
}

// Registers an I/O handle so that the current task gets woken up when it becomes ready.
fn register(cx: &mut Context<'_>, fd: impl AsRawFd, interest: Interest);

// Unregisters an I/O handle.
fn unregister(fd: impl AsRawFd);

@dvc94ch
Copy link
Author

dvc94ch commented Nov 2, 2019

So how would this be used exactly? Are these operations idempotent? But it seems reasonable.

fn poll_send(&self, cx: &mut Context, data: &[u8]) -> Poll<Result<()>> {
    match self.socket.try_send(data) {
        Ok(res) => {
            unregister(cx);
            Poll::Ready(Ok(res))
        }
        Err(ref e) if e.kind() == ErrorKind::WouldBlock => {
            register(cx, self.socket.as_raw_fd(), Interest::Write);
            Poll::Pending
        }
        Err(e) => {
            unregister(cx);
            Poll::Ready(Err(e))
        }
    }
}

@ghost
Copy link

ghost commented Nov 2, 2019

@dvc94ch Almost -- this is closer to what I had in mind:

impl MyIoHandle {
    fn poll_send(&self, cx: &mut Context, data: &[u8]) -> Poll<Result<()>> {
        let mut registered = false;
        loop {
            match self.socket.try_send(data) {
                Ok(res) => return Poll::Ready(Ok(res)),
                Err(ref e) if e.kind() == ErrorKind::WouldBlock => {
                    if registered {
                        return Poll::Pending;
                    } else {
                        register(cx, &self.socket, Interest::Write);
                        registered = true;
                    }
                }
                Err(e) => return Poll::Ready(Err(e)),
            }
        }
    }
}

impl Drop for MyIoHandle {
    fn drop(&mut self) {
        unregister(&self.socket);
    }
}

@Demi-Marie
Copy link

This blocks quinn-rs/quinn#502, which needs ECN support.

@kornelski
Copy link

I need to call send_sas/recv_sas on a UdpSocket. AsRawFd allows doing it only in a blocking fashion, because I have no way of handling WouldBlock properly.

What I'm missing is a public API equivalent of self.watcher.poll_read_with() / poll_write_with().

@wusyong
Copy link
Contributor

wusyong commented Mar 19, 2020

Hello, I somehow got stuck on the same issue like this. Is it going to introduce new interface in #626, or Watcher will be public in the future?

@katyo
Copy link

katyo commented Apr 21, 2020

I implement async serial port interface (like tokio-serial) which needs Watcher too.
What about introduce special feature (say "internals") which exposes some internal APIs for extension implementers. I think this is simple and harmless solution at this time.

@skade
Copy link
Collaborator

skade commented Apr 22, 2020

@katyo Such a flag can easily be considered standard and lead to de-facto standardisation.

@dvc94ch
Copy link
Author

dvc94ch commented Apr 22, 2020

@skade maybe some features need some experience to get right (otherwise you'd have done it by now)? If you want to be pendantic you could impl api v1 in terms of v2 under a compat flag and have a long deprecation period. If you start with something, and let people try it, you might come up with something better?

@skade
Copy link
Collaborator

skade commented Apr 24, 2020

@dvc94ch I'm definitely sympathetic to do that, but the current proposal is essentially "expose the internals raw", which is definitely not acceptable under the current implementation strategy.

@katyo
Copy link

katyo commented Apr 24, 2020

I think we should expose some internals in any case. Particularly, tokio exposes PollEvented struct which is pretty closest to Watcher. Both wraps something which implements Evented trait.
Mio 0.7 has some APIs changes. Particularly Evented trait renamed to Source, but usage still same.
I like async-std so much, I hope that I can use it as fullfeatured tokio replacement at near time.
Unstability of internal APIs isn't so big problem for me.

@Keruspe
Copy link
Member

Keruspe commented Apr 25, 2020

There are discussions about removing PollEvented from tokio 0.3 fwiw, for this very reason

@leo60228
Copy link

leo60228 commented May 7, 2020

Re-exporting smol::Async (or possibly a wrapper with a simplified API?) would probably make more sense after #757.

@dignifiedquire
Copy link
Member

I think if we can have a light wrapper around it, we should be able to expose sth easier with this, yes

@dignifiedquire
Copy link
Member

I wonder if we actually need to reexport anything, using smol::Async should just work with 1.6 as it is right now

@yoshuawuyts
Copy link
Contributor

@dignifiedquire I agree; we probably don't need to -- though I'd still love to find a way to expose the initialized epoll/iocp/kqueue instance. But that's probably a separate issue.

@leo60228
Copy link

Is smol not an implementation detail?

@Sherlock-Holo
Copy link

Sherlock-Holo commented May 21, 2020

I think smol Async is not enough.

I'm trying to write a zero-copy between two TcpStreams with nix splice and os_pipe, I wrap the PipeWriter and PipeReader with Async, then use the method with.

However I'm thinking: if the writing data from pipe to TCP, and TCP buffer is full, it will return EAGAIN in rust it is WouldBlock, but does the Waker will register with TCP? If the Waker only register with the pipe, we won't know when the TCP is ready because we don't call the TcpStream.read, we use its RawFd directly

@ghost
Copy link

ghost commented Sep 15, 2020

I believe this can be closed now because we have async_io::Async and blocking::Unblock. Both of those crates are stable at 1.0.

You can use Async to asyncify things compatible with epoll/kqueue/wepoll, and Unblock to asyncify everything else. As an example, see how async-process implements ChildStdout: https://docs.rs/async-process/1.0.0/src/async_process/lib.rs.html#474-477

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

Successfully merging a pull request may close this issue.