Skip to content

Add blocking functionality to channels #47

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
merged 3 commits into from
Aug 8, 2022
Merged

Add blocking functionality to channels #47

merged 3 commits into from
Aug 8, 2022

Conversation

notgull
Copy link
Member

@notgull notgull commented Jul 14, 2022

As per discussion in smol-rs/async-lock#25, I have added send_blocking and recv_blocking methods to the Sender and Receiver structures, respectively. In order to reduce code duplication, I implemented this by parameterizing the logic found in the Send and Recv futures using a trait that either polls the EventListener or calls wait on it.

Various things I left up in the air:

  • I added private wait() methods on Send and Recv that use the blocking strategy to poll the future to completion. Should these be public?
  • Recv currently implements Stream. It could also implement Iterator using a similar strategy to how I implemented blocking for the main methods. I decided against it, since StreamExt and Iterator share many method names which could be confused. Is there an idiomatic way of going about this?
  • Do I need more tests? Since I piggyback off of the previous logic I didn't think so, but I'm not sure what the general policy is.

Closes #21

@taiki-e
Copy link
Collaborator

taiki-e commented Jul 16, 2022

  • I added private wait() methods on Send and Recv that use the blocking strategy to poll the future to completion. Should these be public?

I would prefer not to provide public wait methods. see also smol-rs/async-task#16 (comment).

  • Recv currently implements Stream. It could also implement Iterator using a similar strategy to how I implemented blocking for the main methods. I decided against it, since StreamExt and Iterator share many method names which could be confused. Is there an idiomatic way of going about this?

I agree with your decision here not to implement Iterator.

FYI, one of the ways to handle such cases is to provides a wrapper type like BlockingReceiver that implements Iterator and a method to convert it from Receiver explicitly. That said, I don't see the need to provide it until someone actually needs it.

@notgull
Copy link
Member Author

notgull commented Jul 25, 2022

Are there any blockers (no pun intended) for merging this?

Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

I feel that the trait-based solution is a bit overkill, but I'm not sure if there are other good alternatives.

So I think this is ready to merge, aside from a nit.

Co-authored-by: Taiki Endo <[email protected]>
@notgull
Copy link
Member Author

notgull commented Aug 8, 2022

The nit is fixed now, want me to squash it?

Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

@taiki-e taiki-e merged commit aa25b95 into smol-rs:master Aug 8, 2022
@taiki-e
Copy link
Collaborator

taiki-e commented Aug 8, 2022

Published in v1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Synchronous send/recv methods
2 participants