Skip to content

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

Merged
merged 12 commits into from
Jun 20, 2025
Merged

refactor!: Make Sender clone #25

merged 12 commits into from
Jun 20, 2025

Conversation

Frando
Copy link
Member

@Frando Frando commented Jun 6, 2025

This makes the Sender be Clone and take &self in the methods. For the remote sender, this is achieved by putting the actual send stream and buffer into a tokio::sync::Mutex. We also ensure that if sending failed on one sender, subsequent sends on cloned senders will fail as well. And if a send 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:

Local bench
RPC seq 128_996 rps
RPC par 1_093_166 rps
bidi seq 4_281_373 rps
Remote bench
RPC seq 11_393 rps
RPC par 60_073 rps
bidi seq 1_480_826 rps
Reference bench
Reference seq 139_889 rps
Reference par 2_784_308 rps

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 either Frando/improve-bench or Frando/clone-sender.

Fixes #23

@Frando Frando changed the base branch from main to Frando/improve-bench June 6, 2025 12:42
@Frando Frando requested a review from rklaehn June 6, 2025 12:44
Base automatically changed from Frando/improve-bench to main June 12, 2025 18:40
@rklaehn
Copy link
Collaborator

rklaehn commented Jun 12, 2025

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.

@rklaehn
Copy link
Collaborator

rklaehn commented Jun 13, 2025

OK, I think the following would be required to make this not a foot gun in the future:

  • if a send future gets dropped before completion, or if a send future returns an io error, the sender becomes poisoned and will return an io error for all subsequent calls.
  • closed for all other clones of the sender will immediately yield true

That way the scenario I am afraid of - sender gets into a non-working state without this being observable, would be solved.

See #26

@rklaehn
Copy link
Collaborator

rklaehn commented Jun 18, 2025

@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?

@Frando
Copy link
Member Author

Frando commented Jun 18, 2025

SGTM! Can do a review of #26 tomorrow if you want, or just go ahead, as you prefer

@Frando Frando changed the title refactor: Make Sender clone refactor!: Make Sender clone Jun 19, 2025
Copy link
Collaborator

@rklaehn rklaehn left a 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.

@Frando Frando merged commit 8db0017 into main Jun 20, 2025
16 checks passed
Frando added a commit that referenced this pull request Jun 20, 2025
#25 made the `spsc::Sender` cloneable, so it is now a multi-producer channel. This renames the module from `spsc` to `mpsc`.

## Breaking changes

* `irpc::channel::spsc` module is renamed to `irpc::channel::mpsc`
Frando added a commit that referenced this pull request Jun 20, 2025
#25 made the `spsc::Sender` cloneable, so it is now a multi-producer channel. This renames the module from `spsc` to `mpsc`.

## Breaking changes

* `irpc::channel::spsc` module is renamed to `irpc::channel::mpsc`
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.

Provide example of how to pass references to Sender instead of cloning
2 participants