Consider an alternate naming scheme for Sink fns

[carllerche]

#747 recently renamed poll_complete -> flush, dropping the poll_ prefix.

I would suggest to do the opposite. Keep poll_flush and rename close -> poll_close.

The primary reason is to reserve flush and close for futures that perform these operations, especially for async / await. This would allow:

await sink.flush();

and

await sink.close();

I have other thoughts on Sink based on experience implementing and using it, but I am assuming that there will be an RFC at some point to go into more depth on the Sink API.

[aturon]

Agreed -- I think this was a case of crossed wires, when we were considering moving away from the poll_ prefix altogether. Not sure what to do for start_send though.

cc @cramertj

[carllerche]

@aturon

I avoided getting into it, however I think that start_send should be updated to not be an async function.

This is what I think Sink should be:

pub trait Sink {
    type Item;
    type Error;

    fn poll_ready(&mut self) -> Poll<(), Self::Error>;

    fn start_send(&mut self, item: Self::Item) -> Result<(), Self::Error>;

    fn start_close(&mut self) -> Result<(), Self::Error>;

    fn poll_flush(&mut self) -> Poll<(), Self::Error>;
}

IMO the current start_send API is pretty unusable in practice. The primary reason is that it is impossible to return the send item in all cases that are non trivial. The poll_ready strategy has been what I've been using and has been working quite well in practice.

Also, in this proposal, the close strategy is transitioned to a fn start_close that is used to internally transition the Sink state to closing, then poll_flush is called until Ready and the sink can be safely dropped.

This proposal is based on how my implementations of Sink have turned out in practice. It also has the nice side effect of not requiring AsyncSink and following the poll_ pattern.

[carllerche]

Oh, and the proposal has no opinion on the naming of start_send and start_close. I don't think these are great names but I"m using them to match what Sink does today.

[aturon]

@carllerche This looks great! I remember back when we were first doing Sink I went through a bunch of iterations, including one that looked a bit like this, and I agree with the upsides you mention.

One question: what guarantees do we expect for start_send following a successful poll_ready?

[carllerche]

Something like

When Sink::poll_ready returns Ready, the next call to start_send will not fail due to "no capacity" or "not being ready".

What is permitted is poll_ready returning Ready then start_send failing due to the sink being closed.

[aturon]

@cramertj @alexcrichton thoughts on this proposal? To me it seems to clean up Sink nicely.

(Granted there's still the borrowing issues @cramertj, but that's why it's a separate crate we can iterate on...)

[alexcrichton]

Seems fine by me!

[cramertj]

+1 from me.

[cramertj]

@carllerche I encountered an interesting case when transitioning some of the Sink combinators in futures-util. Sometimes you don't know in the call to poll_ready whether start_send will be unblocked. For example, in the SplitSink impl, you don't know whether poll_lock will succeed. I handled this by keeping an Option<S::Item> in the Sink, and it will store the value there if the poll_lock fails.

[carllerche]

Yes, the solution to these cases is to either add a single buffer slot or not implement Sink.

[cramertj]

@carllerche After implementing this change, I found that many implementations needed to store do_close state in start_close so that it could be used in poll_flush to close the underlying object once a flush had been completed. How would you feel about amending the API to use poll_close instead of start_close? It would allow us to drop the extra boolean flag in the Sink implementations-- I expect that, in general, clients of a Sink must already know whether the object they're flushing is being closed or not, so it isn't useful for the Sink itself to store that information.

Edit: alternatively, a closing boolean could be passed to poll_flush. This is essentially the same as having two separate functions, but it hints that the implementations of poll_flush and poll_close may share common elements. I think I personally prefer the two-function variation-- WDYT?

[carllerche]

IMO it comes down to defining what each fn means specifically around what happens when you call start_send after calling poll_close / start_close.

In my impls, I have been treating it as an error (as the sink is closed) and the start_send returns an error. To do this, I needed a closing flag either way.

Also, poll_close makes the "transitioning to closed" state change implied, but a bit fuzzy. At what point does that happen? Is it on the first call to poll_close?

Basically, it comes down to whether or not you make the lifecycle of a Sink explicit or not. If the trait says that it is undefined what happens if you call start_send after poll_close, you could probably do away with the boolean flag in a number of cases. If start_send should error after poll_close is called, then you probably need a boolean flag no matter what.

[cramertj]

@carllerche That makes sense. I think my personal preference would be to make the behavior of start_send after poll_close unspecified-- I don't see a strong benefit to guaranteeing some specific behavior in that case.

[aturon]

Closing, this is done.