Skip to content
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

make initial poll loop less aggressive #15

Merged
merged 1 commit into from
Apr 20, 2022
Merged

Conversation

rkuhn
Copy link
Contributor

@rkuhn rkuhn commented Mar 25, 2022

The async fn didn’t actually yield; it does now.
Also report socket closure as an error.

The async fn didn’t actually yield; it does now.
Also report socket closure as an error.
@mxinden
Copy link
Member

mxinden commented Mar 25, 2022

The async fn didn’t actually yield; it does now.

Can you expand on this @rkuhn?

@rkuhn
Copy link
Contributor Author

rkuhn commented Mar 25, 2022

Previously, this was just a loop, calling poll back-to-back with no pause. An async block doesn’t make much sense without an .await inside.

@mxinden
Copy link
Member

mxinden commented Mar 25, 2022

Previously, this was just a loop, calling poll back-to-back with no pause. An async block doesn’t make much sense without an .await inside.

I am still confused. The select! should be polling both futures, yielding Poll::Pending to the executor when no progress can be made, no @rkuhn?

@rkuhn
Copy link
Contributor Author

rkuhn commented Mar 25, 2022

No, select! does not contain .await, so it can’t yield. Using select! makes no sense in an async fn, unless I am missing something big.

@mxinden
Copy link
Member

mxinden commented Mar 25, 2022

No, select! does not contain .await, so it can’t yield. Using select! makes no sense in an async fn, unless I am missing something big.

That would be surprising to me.

If you use the the Tools->Expand-macros feature of the testground below you see a sample Future generated by the select! macro. While very noisy, it does await the generated Future near the end.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e6f20e1507691e1ad6a577499a8e9943

@rkuhn
Copy link
Contributor Author

rkuhn commented Mar 25, 2022

Sorry, you’re right — today was quite tough and I mixed up macro definitions. I still find my version easier to read; semantics of polling and wakers is sufficiently brittle in Rust async that every layer of abstraction on top of it is more risk than gain.

@mxinden
Copy link
Member

mxinden commented Mar 27, 2022

I still find my version easier to read; semantics of polling and wakers is sufficiently brittle in Rust async that every layer of abstraction on top of it is more risk than gain.

Agreed. I will take a closer look next week. Currently on vacation.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This looks good to me. @dvc94ch any comments from your end?

Ok(None) => break,
Err(err) => return Err(Error::new(ErrorKind::Other, err)),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this should be equivalent. is it really easier to read? in the special case that you only need to poll two futures, maybe, maybe not. for polling more futures/streams select! macro is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust async poses an interesting challenge for developers due to the interactions with other language features. Having all .await points exposed and clearly visible in an async scope is the only way to make that manageable — a macro that expands to .await within the current async scope is actively harmful. Imagine having to debug compiler error messages that some faraway type is not Sync?

ErrorKind::BrokenPipe,
"rtnetlink socket closed",
)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change looks fine to me

@mxinden mxinden merged commit adcce22 into libp2p:master Apr 20, 2022
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.

3 participants