Skip to content

Add Sink01As03 compat shim #1364

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

Merged
merged 9 commits into from
Feb 28, 2019
Merged

Add Sink01As03 compat shim #1364

merged 9 commits into from
Feb 28, 2019

Conversation

LucioFranco
Copy link
Member

This adds a compat shim to go from a 0.1 Sink to a 0.3 Sink. This follows, roughly how the stream compat works with the exception that it stores an internal buffer of one item. This is due to the fact that there is no LocalWaker passed to Sink01::start_send, so we can only actually write the item in the sink on the next Sink03::poll_read which gets a LocalWaker.

This PR is related to #1362 and cc @Nemo157.

Copy link
Member Author

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nemo157 should be ready for another look.

@LucioFranco
Copy link
Member Author

@Nemo157 do we want to still move this forward, I know you might be busy so no need to rush but want to make sure this isn't getting too far out of sync if there still is a need for it.

@Nemo157
Copy link
Member

Nemo157 commented Jan 25, 2019

Sorry, I managed to completely forget about this 😦

I skimmed the latest changes, and they look good at first glance. That's a deep match though, so I will try to read it a bit more thoroughly this weekend.

@LucioFranco
Copy link
Member Author

@Nemo157 no worries! I am still around to finish this up, I will address the one comment left, see if I can ease up that match, and rebase against master.

@LucioFranco
Copy link
Member Author

@Nemo157 sorry for the delay, I've cleaned up the poll_close logic and tested it. All seems like its good to go. Thanks.

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@manuels
Copy link

manuels commented Feb 3, 2019

Great job! There is a slight inconsistency: Usually sink comes first in the function name (e.g. SinkExt::sink_map_err(), SinkExt::sink_err_into(). I would suggest to rename compat_sink() to sink_compat().

@zargony
Copy link
Contributor

zargony commented Feb 28, 2019

I think this may need a rebase because of the LocalWaker to Waker rename.

I'd love to see this merged since it's currently a pain to work with websockets without being able to send to them. I ended up using a copy of this PR in my code which works fine so far, but official support for sinks would be great.

@LucioFranco
Copy link
Member Author

@zargony thats for pointing that out, I should fix this.

@Nemo157 and others are we still planning on merging this, its been sitting for a while...

@Nemo157
Copy link
Member

Nemo157 commented Feb 28, 2019

I’m keen to merge it, but don’t have a commit bit. @cramertj is the only active merger as far as I’m aware (maybe it’s worth spreading that responsibility a bit?).

@cramertj
Copy link
Member

cramertj commented Feb 28, 2019

I'm watching and would be happy to merge when this is ready! @Nemo157 we should definitely get you added to the list-- I'll see what I can do about that.

@LucioFranco
Copy link
Member Author

sounds good, give me a few and ill fix conflict issues.

@cramertj
Copy link
Member

@Nemo157 done! you should have merge rights now.

This adds a compat shim to go from a 0.1 Sink to a 0.3 Sink. This follows,
roughly how the stream compat works with the exception that it stores an internal
buffer of one item. This is due to the fact that there is no `LocalWaker` passed
to `Sink01::start_send`, so we can only actually write the item in the sink on the
next `Sink03::poll_read` which gets a `LocalWaker`.
@Nemo157 Nemo157 merged commit 2d1e371 into rust-lang:master Feb 28, 2019
@Nemo157
Copy link
Member

Nemo157 commented Feb 28, 2019

Thanks for this 😄 I should have time in a couple days to update the compat blog post with these changes and finally get that merged.

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

Successfully merging this pull request may close these issues.

6 participants