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

RFC: Stop using std::net types (TcpStream / UdpSocket) #146

Closed
carllerche opened this issue Apr 7, 2015 · 15 comments
Closed

RFC: Stop using std::net types (TcpStream / UdpSocket) #146

carllerche opened this issue Apr 7, 2015 · 15 comments

Comments

@carllerche
Copy link
Member

I'm considering moving away from using std::net types in favor of re-implementing them in Mio. Mio started off using this strategy, then attempted to use std::net types when available. Mio introduced NonBlock<_> in order to differentiate between blocking std sockets and non-blocking std sockets.

I am considering going back to the original strategy in order to achieve a few things.

First, I would like to get mio working on stable. Unfortunately, Error::from_os_error is not stable, which means that it is impossible to create an error value without allocating. This makes me wary of relying on std::net types in the future to be able to support the features that mio wants to.

Second, I have put a lot more though into windows support. My opinions re: windows support have changed over time. I thought that it would be best to leave windows support to a different library, however, I started writing this library and spent a lot of time with the windows IOCP APIs and I believe that it would be possible to support windows equivalently with some minor API tweaks (backwards compatible). However, to do this, all the network types would need to be implemented in mio.

Also, if mio owns all the types, there wouldn't be much use to have NonBlock<_> anymore since all the types in mio could be non-blocking by default, but this is an optional change.

So, the strategy would be:

  • Move TcpStream & TcpListener into mio
  • Stop using std::io::Error in favor of mio::Error
  • Stop using <NonBlock<_>> (optional)
@Hoverbear
Copy link

You forgot UdpSocket in your strategy.

Do you think that the Error::from_os_error issue is something that should be discussed upstream? Surely mio will not be the only library with this concern.

@carllerche
Copy link
Member Author

@Hoverbear I have mentioned it and I believe they will stabilize it, but not until 1.0 Final would be my guess. If std::io::Error was the only issue, it wouldn't be worth changing anything. However, this change would be required to get Mio running on windows. So, I was hoping to postpone the change, but the std::io::Error issue is bringing it up now.

@reem
Copy link
Contributor

reem commented Apr 7, 2015

I would rather stay on nightlies and use the std types with unstable APIs, if that's possible. I don't think it's actually crucial we get this building on stable rust at 1.0, since it is in a highly experimental area of rust development.

@Hoverbear
Copy link

Requiring nightlies after 1.0 would greatly limit mio's audience.

@CraZySacX
Copy link

I like the idea of one library that support the same platforms rust itself supports over using the std types.

@zslayton
Copy link
Contributor

zslayton commented Apr 7, 2015

I'm in favor of this; at the moment lack of Windows support is the only thing keeping me from using mio for a handful of side projects. It would be great to have platform parity with rustc.

@rozaliev
Copy link
Contributor

rozaliev commented Apr 7, 2015

We are using mio in a couple of projects. All of them will stay on nightlies after 1.0, at least until compiler plugins are stable. Windows support is not important for us, but I guess it would be nice to have for some people. (without additional overhead for *nix part).

But there are some things that I would prefer be different. After last mio reform my colleagues asked me a lot of questions about code like this

match sock.read_slice(...) {
                Ok(Some(n)) => { .. },
                Ok(None) => { .. },
                Err(e) => { .. },
            }

It's very confusing and non intuitive. I believe Option is not ideal way of communicating WouldBlock.

Smth like this would be great.

match sock.read_slice(...) {
                Ok(n) => { .. },
                Err(Mio::WouldBlock) => { .. },
                Err(Mio::Eof) => { .. },
                Err(e) => { .. },
            }

Explicit eof instead of n == 0 would be great too.

With Result<_, mio::Error> it will be possible again.

@carllerche
Copy link
Member Author

@rozaliev I thought a bunch about how to communicate WouldBlock, and IMO the issue is that, when working with non-blocking sockets, a WouldBlock case is not actually an error. The reason this matters is, if using try!(sock.read_slice(...)), a WouldBlock would end up returning early from the function.

If all mio sockets are assumed to be non-blocking, NonBlock could be repurposed as it was in 0.2:

match sock.read_slice(...) {
    Ok(NonBlocK::Ready(n)) => { .. },
    Ok(NonBlock::WouldBlock) => { .. },
    Err(e) => { .. },
}

@reem
Copy link
Contributor

reem commented Apr 7, 2015

I personally would prefer if we reused Option, as it reduces imports and is more compatible with other code that already operates on Option values.

@carllerche
Copy link
Member Author

I think I also personally prefer the Option signature, but it is open for discussion :)

Anyway, got Mio running on Rust 1.0 beta w/o these changes, so there isn't a huge rush now. I think version 0.4.0 will target windows support, which will require some of what I proposed initially. I'll try to post a more in depth windows support plan when I have a moment.

@zslayton
Copy link
Contributor

Out of interest, what would TcpStream and friends look like if they were moved into mio? Would it still be possible to read/write to them in a synchronous fashion if needed?

@carllerche
Copy link
Member Author

The versions in mio would probably be hard coded to be non-blocking. However, there could be conversions between mio::TcpStream and std::net::TcpStream and the mio EventLoop could support registering std::net::TcpStream (and others), but only on posix systems.

@jnicholls
Copy link

Would this affect Mio's ability to interoperate with openssl crate's SslStream<S> for example? Actually, is that even possible today for Mio to event on an SslStream<S> type?

@carllerche
Copy link
Member Author

@jnicholls It shouldn't make it harder. I believe it would be possible for Mio to work w/ a non-block SSL stream type. We could discuss it further on the #mio IRC channel.

@carllerche
Copy link
Member Author

I'm going to close this in favor of #155. Trying to keep things clean...

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

7 participants