-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
Add Send bound to streams. #471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, looks good! Thanks @sebpuetz
7ce941b
to
33a87d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! it looks extremely useful!
@billy1624 it seems to conflict with the changes you made to |
Let me check |
Hey @sebpuetz, please check and cherrypick billy1624@4bf35b4 |
I had to dig a bit into the different
QueryStream
implementations and the only reason for!Send
I could find was the builder function passed to the builder in theouroboros
context. Theself_referencing
macro generates a signature on the current release ofouroboros
that allows!Send
constructors:https://github.com/joshua-maros/ouroboros/blob/main/ouroboros_macro/src/info_structures.rs#L265
I forkedouroboros
and added theSend
bound on the constructor future and replaced the crates.io dependency with one pointing at my branch. After fixing up theSend
requirements on theTransactionStream
, a few other places in this crate needed+ Send
bounds.This PR is not meant to be merged, it's mostly for documentation / demonstration of what would be required to get streams working intokio::spawn
context. If you're interested in getting this PR into better shape, we'll have to get into touch with theouroboros
maintainers to figure out if the missingSend
bound is intentional.I just recognized that
TransactionStream::build
can use a synchronous builder since the builder closure doesn't await any futures. That means the change onouroboros
is not even required to enableSend
streams.The first commit here is a solution that keeps the async constructor, the second one just removes the unused future on build