experiment with splitting this actor state a bit more #3095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The RelayActor needs to process all sent datagrams and pass them onto the correct ActiveRelayActor. But it also needs to process a few other inbox messages. The other inbox messages are higher priority however, since they can heal the connected relays. So this adds a separate datagram send queue, so that the inbox is not flooded by datagrams.
This makes sure to send as many datagrams as quickly as possible to the right ActiveRelayActor but at the same time not block the RelayActor itself. This ensures it can still process other inbox items and instruct the ActiveRelayActors appropriately.
…#3068) ## Description This removes an actor by making it a stream. The main (temporary) downside is that a read error no longer shuts down the Conn WriterTasks. This will not be an issue for that long as the WriterTasks are going away next and then the Client can manage this. The RelayDatagramRecvQueue is grown to 512 datagrams. We used to keep this many frames in the per-relay stream. Though that's potentially an awful lot. For datagrams we can assume that they will at max settle close to 1500 bytes each, so this buffer will end up being 750KiB max. That seems somewhat reasonable, though we could probably double it still. There will effectively no longer be a per-relay buffer - other than inside the relay's TCP stream. ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions This does some other renaming, e.g. `ConnReader` is now a `ConnFrameStream` which is a bit more coherent. It also moves a bunch of code around in the `conn.rs` file to give it some more stucture. This makes the diff harder to read then it needs to be. `process_incoming_frame` has not changed and the main other change is that the reader task is no longer created and instead is moved into the `Stream` impl of the new `ConnMessageStream`. This is open for review. Though whether or not we want to merge it is another matter. I'll see for how the next few PRs on top of this look to see what the real impact of the WriterTasks not being shut down is. Maybe we'll end up merging several PRs together before merging to main. But logically this is a nice dividing point to review. ## Change checklist - [X] Self-review. - [X] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [X] Tests if relevant. - [X] All breaking changes documented. --------- Co-authored-by: Friedel Ziegelmayer <[email protected]>
4 tasks
|
No one felt like having architecture discussions 😁 |
sorry, it got lost.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Experiments with splitting the
ActiveRelayActora bit more. Inspired by #3062 (comment)Breaking Changes
Notes & open questions
This doesn't work yet because there's a nasty
Sendissue and I'm not determined enough to fix this yet. But it's a nice place to discuss the architecture a bit.What is nice from this is that the methods on
ConnectedRelaybecome, well, methods rather than taking this&mut stateso that part feels more natural.What would be nice to achieve on top of this is force the
run_sendingstate into the type system as well since currently calling that is rather by convention. That would be the thing that would make this approach really appealing.A totally different approach is to split the actual actor in 3 again:
The main benefit I would hope to achieve is that the concurrency of the send/receive is easier. In the current version keeping receive going while sending is a bit awkward. What it does not simplify is the state change between dialing and connected that the current actor has to juggle.
The main cost is that there would be another channel between the
ActiveRelayActorand the sending sub-actor. The nice thing about this is that theActiveRelayActorcan work with permits, which could be super nice! The hard thing is deciding on the sizing of this channel. But maybe a single-item channel wouldn't be so bad.I do believe this version is worth exploring more, despite the fact it introduces two more actors again (but one one more queue on the send side already discussed). But also I'm keenly aware I'm time-limited and the current version is not bad.
Change checklist