Refactor Sink trait #765

cramertj · 2018-02-14T09:12:40Z

alexcrichton

Looks good to me, thanks! I think some tests are failing though?

I'm a little fuzzy on what the protocol is in terms of how to use a sink, but we can always flesh that out later.

alexcrichton · 2018-02-14T16:09:29Z

futures-sink/src/channel_impls.rs

+/// It will contain a value of type `T` if one was passed to `start_send`
+/// after the channel was closed.
+#[derive(Debug)]
+pub struct ChannelClosed<T>(Option<T>);


How come this was needed?

Previously, it was always the case that an error contained a T. That's not the case anymore, since poll_ready doesn't have a T to respond with (previously, the only function that could error was start_send, which has a T).

Hm could we mirror the Option in the original futures-channel crate as well though? (to avoid the introduction of another type)

Sure-- i debated about this, since it's sort of misleading to the end user, because some functions will only ever return Some or None. In the Sink trait that's not preventable, but it is if you're directly using functions on Channel. I think I agree that the extra complexity isn't worth it, though.

Eh I'd probably be fine either way, I understand now what's going on at least :)

I guess the key question to me is whether we can expect start_send to always be able to give you the value back on an error, in which case the Sink trait should probably continue to use a wrapper around the error type.

cc @carllerche

cramertj · 2018-02-15T00:18:16Z

@alexcrichton I've pushed another commit fixing up futures-util.

Fix rust-lang#751

seanmonstar · 2018-02-16T18:59:13Z

futures-sink/src/channel_impls.rs

    }

-    fn close(&mut self, _: &mut task::Context) -> Poll<(), SendError<T>> {
+    fn start_close(&mut self) -> Result<(), Self::SinkError> {
+        Ok(())


This has confused me in the past. I would expect to be able to try to close a channel from the sender side, even if I have multiple senders. Should this instead try to close the channel, instead of being a noop?

Can you open an issue for this? I'd prefer to keep this PR scoped to just refactoring changes, not behavioral ones.

seanmonstar · 2018-02-16T19:00:29Z

futures-sink/src/channel_impls.rs

+    type SinkError = ChannelClosed<T>;
+
+    fn poll_ready(&mut self, _: &mut task::Context) -> Poll<(), Self::SinkError> {
+        Ok(Async::Ready(()))


Shouldn't this also check the inherent poll_ready? An unbounded channel can still be closed.

If it's closed, it will throw an error in start_send. However, I think I agree with you that it would be good to allow the client to stop producing values in these cases.

(note that to accomplish this, i'll be changing the inherent poll_ready from &mut self to &self)

oh, it seems I can't actually do that, because poll_ready calls poll_unparked internally, which mutates self.maybe_parked.

As a user, I find it surprising that passing tx would let poll_ready tell me it was closed, but &tx wouldn't.

I don't disagree. Can you open an issue for this?

So just to confirm, we're gonna tackle this in a follow-up PR?

Yes, I think that's a good idea.

aturon

LGTM, left a couple of questions but nothing big.

aturon · 2018-02-16T23:55:30Z

futures-sink/src/channel_impls.rs

+/// It will contain a value of type `T` if one was passed to `start_send`
+/// after the channel was closed.
+#[derive(Debug)]
+pub struct ChannelClosed<T>(Option<T>);


I guess the key question to me is whether we can expect start_send to always be able to give you the value back on an error, in which case the Sink trait should probably continue to use a wrapper around the error type.

cc @carllerche

aturon · 2018-02-16T23:57:49Z

futures-sink/src/channel_impls.rs

+    type SinkError = ChannelClosed<T>;
+
+    fn poll_ready(&mut self, _: &mut task::Context) -> Poll<(), Self::SinkError> {
+        Ok(Async::Ready(()))


So just to confirm, we're gonna tackle this in a follow-up PR?

aturon · 2018-02-16T23:58:07Z

futures-sink/src/lib.rs

-
-/// The result of an asynchronous attempt to send a value to a sink.
-#[derive(Copy, Clone, Debug, PartialEq)]
-pub enum AsyncSink<T> {


ahhhhhh so nice

cramertj · 2018-02-17T00:10:44Z

@aturon

I guess the key question to me is whether we can expect start_send to always be able to give you the value back on an error, in which case the Sink trait should probably continue to use a wrapper around the error type.

@carllerche said in #751:

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.

aturon · 2018-02-17T00:11:33Z

@cramertj ok, that's good enough for me :-)

aturon · 2018-02-17T00:11:44Z

Merged!

carllerche · 2018-02-17T17:57:45Z

@aturon @cramertj

yes, it should definitely not be assumed that, once a value is sent, it can always be returned on error. poll_ready lets you have a pretty good idea that the value is accepted, however there is is always the possibility of a race. For example, poll_ready() returns Ready, the channel closes, then start_send is called. In this case, start_send will error.

Now, some Sink implementations are able to always guarantee that the value can be returned on error. In those cases, the implementations are free to include the send value.

enum SendError<T> {
    Closed(Option<T>), // Must be option to handle returning this on `poll_ready`.
    // other variants
}

impl<T> Sink for Sender<T> {
    type Item = T;
    type Error = SendError<T>;
}

This is probably roughly what @cramertj did (but I didn't look in detail).

The implementation can document that T will always be returned when the error is generated from start_send, but the main point is that the decision is left up to the implementation.

cramertj requested a review from alexcrichton February 14, 2018 09:12

alexcrichton reviewed Feb 14, 2018

View reviewed changes

carllerche mentioned this pull request Feb 15, 2018

Having both "sync" and "sink" in the API makes verbal discussion v. confusing #395

Closed

cramertj added 2 commits February 16, 2018 10:01

Refactor Sink trait

65a1183

Fix rust-lang#751

Update futures-util Sink implementations

7117b21

cramertj force-pushed the refactor-sink branch from 2fba527 to 7117b21 Compare February 16, 2018 18:04

seanmonstar reviewed Feb 16, 2018

View reviewed changes

cramertj added 2 commits February 16, 2018 11:32

Fix futures-channel tests

f50e2d2

Fix futures-channel benches

0fba3db

cramertj requested a review from aturon February 16, 2018 20:04

aturon approved these changes Feb 17, 2018

View reviewed changes

aturon merged commit 3e5aa4b into rust-lang:0.2 Feb 17, 2018

cramertj deleted the refactor-sink branch February 17, 2018 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Sink trait #765

Refactor Sink trait #765

cramertj commented Feb 14, 2018

alexcrichton left a comment

alexcrichton Feb 14, 2018

cramertj Feb 14, 2018

alexcrichton Feb 14, 2018

cramertj Feb 14, 2018

alexcrichton Feb 14, 2018

aturon Feb 16, 2018

cramertj commented Feb 15, 2018

seanmonstar Feb 16, 2018

cramertj Feb 16, 2018

seanmonstar Feb 16, 2018

cramertj Feb 16, 2018

cramertj Feb 16, 2018

cramertj Feb 16, 2018

seanmonstar Feb 16, 2018

cramertj Feb 16, 2018

aturon Feb 16, 2018

cramertj Feb 17, 2018

aturon left a comment

aturon Feb 16, 2018

aturon Feb 16, 2018

aturon Feb 16, 2018

cramertj Feb 17, 2018

cramertj commented Feb 17, 2018

aturon commented Feb 17, 2018

aturon commented Feb 17, 2018

carllerche commented Feb 17, 2018

Refactor Sink trait #765

Refactor Sink trait #765

Conversation

cramertj commented Feb 14, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Feb 17, 2018

aturon commented Feb 17, 2018

aturon commented Feb 17, 2018

carllerche commented Feb 17, 2018