Skip to content

[docs] clearer variable name for broker_handle #100

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
1 commit merged into from
Aug 24, 2019

Conversation

zvkemp
Copy link
Contributor

@zvkemp zvkemp commented Aug 22, 2019

  • use broker_handle as a variable name to avoid overloading the broker function
  • keep the Result from client_writer in writers (makes the loop with writer.await?; work correctly)
  • return Result from brokers in the final code example

@zvkemp zvkemp changed the title clearer variable name for broker_handle; keep result of client writer [docs] clearer variable name for broker_handle; keep result of client writer Aug 22, 2019
@zvkemp zvkemp force-pushed the clean-shutdown-doc-fixes branch from 853cc9e to 8bc58cf Compare August 22, 2019 22:44
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thank you! I have just one request for a change :)

@@ -60,7 +60,7 @@ async fn broker(mut events: Receiver<Event>) -> Result<()> {
Entry::Vacant(entry) => {
let (client_sender, client_receiver) = mpsc::unbounded();
entry.insert(client_sender);
let handle = spawn_and_log_error(client_writer(client_receiver, stream));
let handle = client_writer(client_receiver, stream);
Copy link

Choose a reason for hiding this comment

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

I think it'd be best to still spawn the task here, otherwise futures will not execute in parallel.

If we just collect these futures into a Vec and then await them at line 72, they will execute serially. But if we spawn them first, they will execute in the background on other threads, and at line 72 we just await these tasks to complete.

@zvkemp zvkemp force-pushed the clean-shutdown-doc-fixes branch from 8bc58cf to d22ad68 Compare August 23, 2019 17:25
@zvkemp
Copy link
Contributor Author

zvkemp commented Aug 23, 2019

Thanks @stjepang . Looks like one issue was addressed in another PR; I've rebased and reduced this to a single change.

@zvkemp zvkemp changed the title [docs] clearer variable name for broker_handle; keep result of client writer [docs] clearer variable name for broker_handle Aug 23, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks great, thanks :)

@ghost ghost merged commit 6d6328a into async-rs:master Aug 24, 2019
This pull request was closed.
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.

1 participant