-
Notifications
You must be signed in to change notification settings - Fork 111
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
feat(common): bytes::Buf wrapper that notifies subscribers on EOS #6
base: master
Are you sure you want to change the base?
feat(common): bytes::Buf wrapper that notifies subscribers on EOS #6
Conversation
70f5ae4
to
0ec027e
Compare
I did not understand the behavior of |
0ec027e
to
c004653
Compare
r? @seanmonstar (I wish that worked 😄) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that some may want this util on it's own. I think more importantly though, we should keep the end goal in mind, and "work backwards". That is to say, I suspect most often people will want a channel where they can do tx.flush().await
. We don't need the full impl to start with, but a full public API design/proposal would help us see what the user would write.
We want to ensure that the notifier is only called once, when writing that earier I tried to force the notifier into an Arc<Option<...>>, completely forgetting about the ownership-safe and easier Option<Arc<...>>.
fd61c49
to
9150c0f
Compare
Are you suggesting we write the proposal before continuing? I'll be honest, I was thinking about my use case so far. I think your example (although I'm fuzzy on the details) would work as well, I can imagine some let tx = SomeChannel::new();
let buf = Bytes::from_static(b"abc");
let (buf, signaler) = NotifyOnEos::new(buf);
{
let tx = tx.clone();
tokio::spawn(async move {
signaler.wait_till_eos().await;
tx.flush().await;
}
}
while buf.remaining() > 0 {
tx.send(buf.chunk());
buf.advance(buf.chunk().len());
} |
I don't think it's a strong blocker, but rather a very helpful tool to make sure we solve the problem from the user's point of view. I just currently am focused on things that must be done for hyper's 1.0 release candidate, and coming up with this proposal is not one of those things yet.
That's reasonable! It's certainly an user's point of view, which is better than my hand-wavey idea that I jotted down in the issue to solve an abstract problem I've heard many voice. It does sound like having this util to wrap any |
How would one use this, practically speaking? |
Closes hyperium/hyper#2858.