Combine message box receivers #1738
Merged
Conversation
This file contains 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
jmshark
commented
Feb 20, 2023
jmshark
commented
Feb 20, 2023
albinsuresh
reviewed
Feb 21, 2023
| // TODO: rename | ||
| #[async_trait] | ||
| trait Foo<Input> { | ||
| async fn a(&mut self) -> Result<Option<Input>, RuntimeRequest>; |
There was a problem hiding this comment.
Suggested change
| async fn a(&mut self) -> Result<Option<Input>, RuntimeRequest>; | |
| async fn try_recv(&mut self) -> Result<Option<WrappedInput>, ChannelError>; |
Else how will we propagate any channel specific errors? For those actors not wanting to handle the RuntimeRequest, the c variant is already there. So, I don't see any issues in this being wrapped.
There was a problem hiding this comment.
The point of this fn is the ergonomics of handling the RuntimeRequests as err. So I think Result<_, Either<ChannelError, RuntimeRequest> might be better.
CombineReceiver never produces Channel errors so I guess there's some message box impl where this would be useful, could you point me to an example where this would be useful?
There was a problem hiding this comment.
Okay I just noticed that the recv method does not return any ChannelError but only an Option. It was only send that returns ChannelError. I guess any errors while recving from a channel is implicitly mapped into Option:None instead of returning that error. So, this signature is fine.
jmshark
force-pushed
the
impr/combine-mailbox-receiver
branch
2 times, most recently
from
7acabfc
to
25cfaf1
Compare
jmshark
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
Comment on lines
190
to
191
| // TODO: Should be removed | ||
| pub async fn recv(&mut self) -> Option<Input> { |
There was a problem hiding this comment.
Why not remove it now as we have impl<I,O> ReceiveMessages<I> for SimpleMessageBox<I,O>
|
|
||
| pub struct CombinedReceiver<Input> { | ||
| name: String, | ||
| logging_is_on: bool, |
There was a problem hiding this comment.
For logging, we need to go away from this logging_is_on stuff. This has been done in a hurry - my bad.
With this new trait ReceiveMessage it will nicer to have LoggingReceiver that owns an inner receiver and logs the messages.
Comment on lines
304
to
305
| self.logging_is_on = on; | ||
| self.receiver.logging_is_on = on; |
There was a problem hiding this comment.
I see no point to have this twice. This has to be refactor anyway.
Comment on lines
420
to
435
| #[async_trait] | ||
| impl ReceiveMessages<ConfigInput> for ConfigManagerMessageBox { | ||
| async fn try_recv(&mut self) -> Result<Option<ConfigInput>, RuntimeRequest> { | ||
| self.input_receiver.try_recv().await | ||
| } | ||
|
|
||
| async fn recv_message(&mut self) -> Option<WrappedInput<ConfigInput>> { | ||
| self.input_receiver.recv_message().await | ||
| } | ||
|
|
||
| async fn recv(&mut self) -> Option<ConfigInput> { | ||
| self.input_receiver.recv().await | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Having to repeat ourselves, here, is a sign that this actor should use a SimpleMessageBox.
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Sharkey <james.sharkey@softwareag.com>
jmshark
force-pushed
the
impr/combine-mailbox-receiver
branch
from
c85302a
to
d55f262
Compare
jmshark
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
approved these changes
Mar 6, 2023
Comment on lines
-131
to
-132
| // FIXME: If I add the RuntimeRequests here and if the channel we use to send messages is dropped then we will get an ChannelError::SendError | ||
| // FIXME: but I don't think we shouldn't return this error if the message box has a shutdown message for us |
There was a problem hiding this comment.
I would not bother too much for that. Definitely not for this example, but even for most actors. That said, handling correct termination during shutdown can be tricky when some actor expect other peers to say up a bit longer.
|
|
||
| /// Returns [Some] message the next time a message is received. Returns [None] if | ||
| /// both of the underlying channels are closed. | ||
| /// Returning [RuntimeRequest] takes priority over messages. |
There was a problem hiding this comment.
This comment has to be updated as currently returning None on RuntimeRequest.
|
|
||
| async fn recv(&mut self) -> Option<Input> { | ||
| self.input_receiver.recv().await.map(|message| { | ||
| self.log_input(&message); |
There was a problem hiding this comment.
Okay for now - as this keeps the current behavior.
However, as a follow up PR, we will need to log whatever the recv method. The simpler will be to use a LoggingReceiver which wraps a CombinedReceiver, implements ReceiveMessages and logs received messages.
Comment on lines
+44
to
+46
| let (publish_sender, publish_receiver) = channel(10); | ||
| let (signal_sender, signal_receiver) = channel(10); | ||
| let input_receiver = CombinedReceiver::new(publish_receiver, signal_receiver); |
There was a problem hiding this comment.
An improvement could be to add a CombinedSender type along a chanel() that create a sender, receiver pair.
This combined sender will have to impl RuntimeRequestSink to ease the implementation of specific actor builders.
jmshark
force-pushed
the
impr/combine-mailbox-receiver
branch
from
d55f262
to
9e75197
Compare
jmshark
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jmshark
force-pushed
the
impr/combine-mailbox-receiver
branch
from
9e75197
to
e042ad1
Compare
jmshark
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
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.
Proposed changes
Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments