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

Add the Close trait #2677

Open
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
@czipperz
Copy link

commented Apr 4, 2019

Rendered

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

For some prior-art in the Rust ecosystem Tokio has AsyncWrite::shutdown and Futures 0.3 has AsyncWrite::poll_close that have a similar function. One way that these differ from the proposed trait is that they are explicitly only associated with the writer side of a potentially duplex IO stream. This becomes interesting when you consider something like TcpStream where you may want to close your outgoing data but continue receiving data until the other side also closes their stream.

@CryZe

This comment has been minimized.

Copy link

commented Apr 4, 2019

I think this could also be generalized to a general TryDrop similar to TryFrom and TryInto.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

@CryZe As it stands, Close is a generic way to try dropping an object. I thought about renaming it to TryDrop, but decided on Close because it is more consistent with other programming languages' features. However, we could have a method close on File and other resources that simply try_drops them.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

As an aside, I was unsure of whether Close should be default implemented for all types. TryDrop would also seem to be in favor of defining a default implementation for any type implementing Drop by simply returning Ok(()), similar to how TryFrom and TryInto work. But this implementation is fundamentally wrong -- it ignores these errors.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 5, 2019

Close also has the signature fn close(self) -> Result<(), Error>. But Drop takes &mut self. That is, it isn't a direct generalization from Drop to Close because the signatures are different. I'm proposing a library solution here, not a language solution.

A language solution could take &mut self and call the try_drop with ?, cascading errors. This would, however, require a large breaking change to the language and I am not in favor of this.

czipperz added some commits Apr 8, 2019

Fix double flush issue in `BufWriter` implementation of `Close`
When we call `self.into_inner()`, it internally calls `self.flush_buf()`.  If
flushing causes an error, then the stream shouldn't be flushed again.  Thus we
must turn off the `self.flush_buf()` call in the destructor.  Either
`self.panicked = true` or `self.inner.take()` works.  I choose the second
because it more explicitly shows we are ignoring errors in dropping the inner
writer.
@WiSaGaN

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

What is the benefit of adding a new trait rather than just adding as an inherent method?

@Ixrec

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@WiSaGaN The RFC gives the examples of close() methods on BufReader<T> and BufWriter<T> that would not be possible to write if they couldn't be generic over the closeable T they're buffering.

I do think the RFC would benefit from calling that out early in the motivation; I had the same question until I got to those examples.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

This is at the end of the first paragraph of motivation. Do you have any other concerns @WiSaGaN ?

This will also allow us to close wrapper types, such as BufReader or BufWriter when they wrap any type implementing Close.

@czipperz czipperz force-pushed the czipperz:close-trait branch from cd4ffdc to 3531f18 Apr 8, 2019

@czipperz czipperz force-pushed the czipperz:close-trait branch from 1d277a3 to 5c08776 Apr 8, 2019

@vi

This comment has been minimized.

Copy link

commented Apr 10, 2019

Does it need to be self? Is &mut self not enough to close a resource?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@vi if you want to statically ensure no one can use the resource after it’s closed, then you need to take self, not &mut self

@casey

This comment has been minimized.

Copy link

commented Apr 10, 2019

Nice RFC!

I've wanted something like Close in the past, and one thing that I also would have used is an annotation that tells the compiler that it should emit a warning or error if a type that implements Close is not manually closed, i.e. something like MustClose. In my particular case, I had a resource that might throw an error on cleanup, and I wanted to make sure that I always cleaned up instances of those resources and handled the errors appropriately. Having the compiler warn me whenever it was inserting automatic drops fo these resources (and thus not handling errors) would have been very handy.

I personally would find it a bit confusing if all types that implemented Drop automatically got implementations of Close, since it would look like cleaning up those resources was fallible. However, it might be useful for generic code, and it might have type Result<(), !>, indicating that it never fails. Still, I would hold off until there's a need.

I think I like TryDrop a bit more than Close. I'm imagining resources other than files, things like debugging aids, benchmarking aids, etc, and close() doesn't necessarily make sense for those.

Since I think it generalizes to non io resources, it would be intuitive to me somewhere other than io, perhaps mem.

@vi

This comment has been minimized.

Copy link

commented Apr 10, 2019

@vi if you want to statically ensure no one can use the resource after it’s closed, then you need to take self, not &mut self

@pnkfelix How do I use Close::close from tokio_io::AsyncWrite::shutdown then? Also how do I close the Box<dyn TraitThatExtendsClose>?

Maybe &mut self version of the trait should be listed in alternatives in the RFC? Being not object-safe/dyn-capable is not nice.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

@vi It already does have it listed in the alternatives. I'll add the dyn problem to it though

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

@vi how do we deal with this issue now with just normal Drop implementations? It would have to do something like call ptr::drop_in_place, which is unsafe.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

Mmh. It seems shutdown and the like prepare the object for Drop to be called. I'll put that in the prior art section

@matklad

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@burdges

This comment has been minimized.

Copy link

commented Apr 11, 2019

If I understand, there are many designs suitable for different settings, but this one fits with std::io, right?

We'd manually propagate errors with this by calling .close() ?, right? I suppose you considered automatically propagating errors with roughly

pub trait Drop
    type Error = !;
    try_drop(?? self) -> Result<(),Error> { self.drop(); Ok(()) }
    drop(&mut self);
}

Actual drop glue would call Drop::try_drop(..) ? resulting in type errors if you lack the correct conversion impls though, requiring a truly massive breaking change, which sounds unacceptable. It goes against Rust's explicitness goal too I guess.

I'd think transactional data structures require multiple &mut self functions that confirm or above transactions, but those sound custom and unlikely to fit anything std provides. I doubt ending session types could be homogenized enough to fit into std but..

/// Try ending a session type
pub trait TryEnd {
    type Error;
    fn close(self) -> Result<(), (Error,Option<Self>)>;
}
impl<T: TryEnd> Drop for T {
    drop(&mut x) { panic!() }
}

I think this RFC's design makes sense, but the documentation should explain that Close exists primarily for std::io, and that it may or may not fit other uses cases, so users should not attempt to shoehorn every type into Close.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

@burdges

pub trait Drop
   type Error = !;
   try_drop(?? self) -> Result<(),Error> { self.drop(); Ok(()) }
   drop(&mut self);
}

This won't work for the following reasons:

  1. drop cannot be manually called. Thus try_drop isn't writable.
  2. try_drop shouldn't be manually called in this instance for the same reason
  3. How do we recursively try_drop while we can't manually call try_drop? For instance, how does a BufReader implement this?
```

Although this checks if there were errors in writing the data to the file, there
sometimes are spurious failures that aren't caught here. Our program is

This comment has been minimized.

Copy link
@scooter-dangle

scooter-dangle Apr 12, 2019

For someone like myself who might not be familiar with them, can you give some examples of what these failures might be? Are there any that File::sync_all wouldn’t surface?

This comment has been minimized.

Copy link
@czipperz

czipperz Apr 14, 2019

Author

For specifically File, that is correct. sync_all allows us to handle these errors. Maybe a better example is something like a TCPStream where you have to send a finish packet to signal to the other side to close the connection. (At least that is my understanding from googling tcp for a few minutes)

@xfix

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

About alternatives, I'm wondering about adding a close method to std::io::Write trait.

fn close(mut self) -> io::Result<()> {
    self.flush() // Or `Ok(())` I suppose, this is arguable
}

The RFC argues this is a bad idea as "it would remove backwards compatibility", but I don't see it myself being a case. A default implementation would mean that the already existing structures would work, sure, maybe they wouldn't close the file, but it would be good enough in many cases. One interpretation I could think of is that code explicitly implementing close would not work in Rust versions before close introduction, but so would be the case whatever we would go with. Could this be clarified?

It also argues that "It also prevents us from closing reading resources", which frankly seems like non-issue, what does close failing for a structure implementing Read and not Write would even mean - there were no changes, so surely an error could be ignored - everything what was needed was done with a file, bytes were retrieved without errors. In theory, close can always fail, but I'm not sure what reasonable implementation would return an error other than EINTR (which is not really an error) on close if a file was never written to. A Drop implementation should be sufficient here.

As for other traits where closing can error, I think it makes sense for those traits to provide their own close methods. Having Rust defined Close trait isn't really advantageous compared to traits providing their own close method.

Keep in mind that having Close trait would mean changing <T: Write> to <T: Write + Close> would break backwards compatibility (unless we would have a default implementation for Close), as not every Write implementation will implement Close (if say, due to being written before Close existed).

One issue with Write having close method is that Box<dyn Write> would get a bad close implementation due to close method not being object safe. I have no suggestions on how to deal with this issue. It's possible to have an implementation detail like fn close_box(self: Box<Self>) -> io::Result<()> { (**self).close() } specifically to let close on Box<dyn Write> work, but that's non-ideal. Alternatively, it's possible to make close take a &mut self parameter, but that would cause practical issues with implementing the close method.

@czipperz

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

@xfix Thanks for the feedback!

I was arguing that it would be backwards incompatible if we didn't have a default implementation. That is what Tokio and futures-preview does (see the comment by Nemo above). That is, they require an implementation of shutdown or poll_close.

I think it makes sense just to flush and auto drop. I'll add this in.

dyn Write

To be perfectly honest, it's unclear to me what we should do about this case. Because when we close a resource we should take it by value and dispose of it. But at the same time, dynamic types can't work with this. The approach taken by both Tokio and futures-preview is to take &mut self. I don't particularly like this implementation because it puts the stream into an invalid state and it is unclear what to do besides drop it at that point.

@xfix

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Actually, Box<dyn Write> should be non-issue, now that Box<dyn FnOnce()> works in Rust 1.35, we could do the same thing for already existing Box<T> where T: ?Sized + Write implementation making use of unsized_locals feature (considering we already do for FnOnce(), doing that for Write as well should be non-issue).

Other dynamic dispatch types are non-issue, because they cannot pass the ownership, Box<dyn Write> was the only one I was concerned about. Using close on &mut T will work, but I think there could be a Clippy lint for that, similar to already existing drop_ref lint - it's non-ideal that it will work, but cannot really do anything about that. But hey, if you pass &mut T to a function expecting a Write implementation, maybe you don't want the file to be actually closed.

@xfix

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Question, let's say close method gets added to Write, what should be the implementation for &mut T where T: ?Sized + Write? Specifically, should it be flushing or not. I can imagine a popular use-case of close will be to properly clean up a value instead of implicitly dropping in generic functions accepting Write implementations, and for &mut T, you may not want a drop because the value is still alive, in which case flushing is probably unnecessary.

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

IMO if Write::close exists (and is defined over &mut Self) then impl Write for &mut impl Write + ?Sized should be proxying it through correctly, if you want to pass a borrowed Write implementation to a function and not have it close it (or not flush it) you can wrap the &mut Write in a type that will proxy through all other functions and ignore calls to close (or flush).

@xfix

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@Nemo157 fn close(self) takes ownership, so &mut T implementation cannot redirect to the inner close implementation.

In my opinion, doing nothing (returning Ok(()) is reasonable here). I'm assuming people may want to update their generic APIs to explicitly call close instead of dropping the value, so it makes sense to avoid flushing.

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@xfix it doesn't have to take self by value, e.g. TcpStream::shutdown takes &self so you can call that and acquire a Write instance that is already closed.

I'm assuming people may want to update their generic APIs to explicitly call close instead of dropping the value, so it makes sense to avoid flushing.

That depends a lot on the specific API, if it's just a helper API then it shouldn't be calling close (or flush). If it is an API that expects to take full ownership and manage the lifetime of the data stream, including closing it once complete, then not flushing it could cause issues, e.g. if it sends a final small message, closes the writer, then waits for an ack on a reader; if closing the writer doesn't flush out the final message (i.e. the writer were a &mut BufWriter) this would sit waiting on the ack forever.

@tanriol

This comment has been minimized.

Copy link

commented Apr 18, 2019

@czipperz Please note that both Tokio's shutdown and Futures's poll_close have one significant difference in semantics from the suggested Close trait. Both are poll-style async, so if they return NotReady you're supposed to call them again after they're ready to make progress, possibly more than once.

This is actually a known problem that completion-based IO operations may have no way to cancel them synchronously and thus Drop does not work with them... Close, as defined now, will not work either. This does not feel good, especially taking into account that these are just the kind of operations where one can expect an error during shutdown.

@xfix

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Closing asynchronous I/O is out of scope for this RFC in my opinion. It's so different to regular closing that even if we somehow made Close work for both synchronous and asynchronous code, the bounds for functions using Close trait would be so different that there would be no reason for doing so.

I also feel similarly about Close trait being general, and I strongly would prefer a close method in io::Write. I don't see users ever just going <T: Close>, it would be <T: Write + Close<Error = io::Error>> most of the time. If any other trait would like to have a closing abstraction, it should provide its own - there is no real way to make Close practically general. If most implementations of a trait don't require closing, having a Close trait isn't an improvement, as it would require an user to put a Close bound while implementations of a trait wouldn't implement Close, and often enough this bound wouldn't be put, because most implementations wouldn't require closing. Additionally, having a single Close trait would prevent having multiple Close implementations with different error types.

When having a close method, the implementations could look like this.

  • File - self.sync_all()
  • Cursor<Box<[u8]>>, Cursor<Vec<u8>>, Cursor<&mut Vec<u8>>, Cursor<&mut [u8]> - default (no-op)
  • Sink - default (no-op)
  • Stdout, Stderr, StdoutLock, StderrLock - default (flush) (stdout and stderr can be closed, but it's a bad idea, probably unsafe, and would lead to bugs)
  • TcpStream, UnixStream - self.shutdown(Shutdown::Both)
  • ChildStdin - default (flush)
  • Vec<u8> - default (no-op)
  • File - self.flush()?; self.sync_data() (I think self.sync_all() would be unnecessary here)
  • &mut W - default (flush)
  • &mut [u8] - default (no-op)
  • Box<W: Write> - close inner (using unsized_locals feature)
  • BufWriter<W>/LineWriter<W> - flush buffer to inner and then close inner
  • &File, &TcpStream, &UnixStream - no idea? just flush? Keep in mind that while those can shutdown or sync data, &mut File uses a &mut W implementation (which cannot close due to not having ownership) and it would be weird if &File did sync while &mut File didn't.

As mentioned, poll_close polls, which is why it cannot really require ownership. Meanwhile close due to not polling can require ownership.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.