Change the UnixStream interface so that close_write on a variable preven...

[jbapple]

...ts (at compile time) future attempts to write to that variable.

This is also true for close_read.

Note that UnixStreams cloned from a variable on which close_* is called are not affected by this change. They have the same interface as before, in which reads and writes may fail due to the underlying stream being closed for that operation.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

[rust-highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[alexcrichton]

This is an interesting idea, thanks for the PR! I have a few concerns with this approach, however:

• Is there a reason that you only applied this to UnixStream instead of TcpStream as well?
• I'm not sure this truly detects usage such as this at compile time. Can't you continuously close the read/write halves, getting back the other half?
• The timeout support for I/O objects will likely be either overhauled or #[experimental] after stabilization occurs, so it may be best to hold off on tweaking the design to that point.

[jbapple]

Q: Why not TcpStream, too?

A: I wanted to see if this approach works well first, in no small part based on feedback on this PR.

Q: Can't you seesaw back and forth, getting back your read abilities by calling close_write after close_read?

A: I don't think so. Calling close_read on a UnixStream returns a UnixWriteStream. After you've done that, you can call close_write on the UnixWriteStream, but then you get back a () - see lines 96 and 97: impl WriteStream<()> for UnixWriteStream { fn close_write(self) -> IoResult<()> {.

Q: Should we hold off due to timeout issues?

A: I suspect this change and any timeout changes would interact in a commutative and rather mechanical way, not requiring much thought.

[alexcrichton]

Ah I see how it work! I got lost in the trait impls, clever though!

This does have the unfortunate drawback that the API of UnixStream must be duplicated among the stream, reader, and writer handles. The API as-is today is quite small, but you may find that TcpStream has a much larger api to tweak various settings. I would also expect UnixStream's API to grow over time.

It may be best to punt this sort of static guarantee to an application writer rather than the standard library itself. I personally expect the close_foo methods to be callable if the stream were, for example, stored in a structure. Requiring consumption means that the field has to be moved in and out with an Option, for example.

[jbapple]

The duplication is unfortunate, thought relatively small, since the UnixReadStream and UnixWriteStream are just delegating to UnixStream. Is there some more clever way to remove that duplication using trait bounds or generics or the public/private distinction?

As far as the problems with storing these in a structure goes, this is roughly the same thing one gets from the listen function on Listener, which also consumes its argument, no? A quick grep show this is also the case with

• GenericPath::into_vec
• UdpStream::disconnect (which gives back a UdpSocket)
• TmpDir::unwrap (which gives back a Path)
• several functions on Results (and_then, err, ok, map, etc.)
• EmptyBucket::put (which returns a FullBucket)
• TaskBuilder::{try,try_future} (which give back Result and Future)

[alexcrichton]

Many methods do indeed operate on self, but they're generally not intended for stored structures and don't necessarily apply. The examples you gave are all designed to be a consuming operation as that's inherent to the design of the type itself. For example:

• into_vec follows the convention that consumption methods are prefixed with into, this is intended to reacquire ownership of the underlying byte vector of a Path.
• disconnect is a bit of an outlier and is not necessarily idiomatic design, it has not been scrutinized in terms of stabilization.
• unwrap (now named into_inner), follows the into convention to relinquish ownership of the underlying path
• Result and Option are designed to have almost all operations work on self
• EmptyBucket::put is never stored in a structure, it's only used in temporaries
• TaskBuilder::{try, try_future} are used as part of the builder consumption step. This is how the builder API is designed for TaskBuilder.

Here this is just an I/O operation which does not actually need to consume the stream by any means.

[jbapple]

I can see that it might require more code to manage the different types of streams in one structure, though I view this cost as worth it. I can appreciate that this view is not the only possible one.

Do you have any thoughts on how this sort of static guarantee can be maintained while lessening the burden on a programmer who wants to keep some closed and some open streams in one structure without decorating them as such?

[jbapple]

I could add a type that exposes an interface similar to the current one in which close_* do not consume the self object. Certainly such a thing could be written by a user without access to the internals, but adding it could make the library use more convenient for those who track the streams in containers and call close_* on some of them.

[alexcrichton]

I don't personally have a great idea in mind for how to get this sort of static guarantee, but I'll reference my comment on your other PR for pipes to stress that we need to consider the design of std::io as a whole, not one module at a time. We'll be getting to this soon, and there will likely be an RFC on this topic, and we'll definitely be sure to cc you to make sure it looks good to you!

[jbapple]

Sounds good to me!

[jbapple]

Here's an implementation for TCP streams that uses generics rather than traits:

https://github.com/jbapple/rust/compare/affine-tcp?expand=1

I'm not submitting a PR for this at the moment, in order to wait on the std::io RFC you referenced.

[alexcrichton]

@aturon and I have had taken quite some time to look at std::io this week in order to prepare for a stabilization RFC. Our proposal will deviate quite significantly from today's design, and I think that this PR may not be so relevant afterwards. For now to reduce the load on @bors I'm going to close this, but I'll try to keep you posted @jbapple!