-
Notifications
You must be signed in to change notification settings - Fork 3
refactor!: Make Sender
clone
#25
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
Conversation
Still not a fan. Imagine you clone a sender. Then on one clone you send a large value, and that fails in the middle. The other sender now has no idea that the send failed, and will happily try to send as well. Now we got half a value on the wire, and the sending will continue to work, but the other side won't be able to deserialise anything anymore. I think if we want to do this we would have to poison the sender after the first io error. But it is becoming some kind of rube goldberg machine. |
OK, I think the following would be required to make this not a foot gun in the future:
That way the scenario I am afraid of - sender gets into a non-working state without this being observable, would be solved. See #26 |
if the future gets dropped before completion or if it returns an io error.
@Frando I thought about this some time and I think we should do it after all. One of the main design goals of irpc is to make the in-mem case as convenient and free of overhead as possible, so you can use irpc to do optionally networked services. And since we are wrapping the tokio mpsc channel, it seems not nice to take away something important from the in memory case just to accomodate the rpc case that we might not even care about that much in some cases. So WDYT? Merge #26 and then merge this and rename spsc to mpsc? |
SGTM! Can do a review of #26 tomorrow if you want, or just go ahead, as you prefer |
Make sure sending fails after the first io error or future drop
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.
With the changes to make this brute force cancel safe, I think it's fine. I am still a bit confused why adding a lock makes things faster, but 🤷 .
We should rename the channel to mpsc at some point, but not in this PR.
This makes the
Sender
beClone
and take&self
in the methods. For the remote sender, this is achieved by putting the actual send stream and buffer into atokio::sync::Mutex
. We also ensure that if sending failed on one sender, subsequent sends on cloned senders will fail as well. And if asend
future is dropped before completion, further sends will fail as well, as we cannot guarantee integrity of the send stream anymore if a send future was dropped.Breaking changes
The methods from
Sender
all take&self
instead of&mut self
.Notes and open questions
My quick benchmark shows some weird numbers:
Compared to #24 the
bidi seq
for remote got much faster. I can't really explain that? But I ran quite a few times, without much change on eitherFrando/improve-bench
orFrando/clone-sender
.Fixes #23