Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

overseer: send_msg should not return an error #1995

Merged
merged 4 commits into from
Nov 23, 2020
Merged

Conversation

ordian
Copy link
Member

@ordian ordian commented Nov 22, 2020

@ordian ordian added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 22, 2020
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Nov 22, 2020
@ordian ordian marked this pull request as ready for review November 22, 2020 21:24
@ordian ordian added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 22, 2020
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

node/overseer/src/lib.rs Outdated Show resolved Hide resolved
node/overseer/src/lib.rs Outdated Show resolved Hide resolved
where T: IntoIterator<Item = AllMessages> + Send, T::IntoIter: Send
{
let mut msgs = stream::iter(msgs.into_iter().map(ToOverseer::SubsystemMessage).map(Ok));
self.tx.send_all(&mut msgs).await.map_err(Into::into)
if self.tx.send_all(&mut msgs).await.is_err() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending can only fail when the overseer has closed the channel. Do we really need to log this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be helpful for debugging purposes when running tests. Normally we should not see it fail when if the overseer sends Conclude signal in test environment. I can downgrade the log level to debug though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, ty

}

impl<M> OverseerSubsystemContext<M> {
async fn send_logging_error(&mut self, msg: ToOverseer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird naming

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions? :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe send_and_log_error?


impl<M> OverseerSubsystemContext<M> {
async fn send_logging_error(&mut self, msg: ToOverseer) {
if self.tx.send(msg).await.is_err() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

node/overseer/src/lib.rs Outdated Show resolved Hide resolved
node/overseer/src/lib.rs Outdated Show resolved Hide resolved
ordian and others added 2 commits November 23, 2020 11:11
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
@bkchr bkchr merged commit ffedeab into master Nov 23, 2020
@bkchr bkchr deleted the ao-cleanup-error-handling branch November 23, 2020 11:42
ordian added a commit that referenced this pull request Nov 23, 2020
* master:
  overseer: send_msg should not return an error (#1995)
  Bump async-trait from 0.1.41 to 0.1.42 (#1996)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subsystems should be resilient to errors
3 participants