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

Use tokio-stream crate to replace usages of Tokio types that were Streams #2922

Closed
wants to merge 3 commits into from

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Oct 20, 2021

Motivation

This is part of the update to use Tokio version 1 (#2200), and must be merged together with the other PRs.

Newer versions of Tokio have removed some impl Stream from some types, so they have to either be used differently or have them wrapped in helper types from a separate tokio-stream crate.

Solution

Wrappers from the tokio-stream crate were used to fix a few usages of Tokio types that were previously Streams.

Review

Anyone from the team can review this, but it shouldn't be merged yet.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@jvff jvff requested a review from a team October 20, 2021 22:55
@jvff jvff self-assigned this Oct 20, 2021
@mpguerra mpguerra mentioned this pull request Oct 29, 2021
10 tasks
@mpguerra mpguerra requested review from oxarbitrage and removed request for a team October 29, 2021 14:07
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this looks fine and i can approve, however is hard to do so while depending on others pieces. We need to at least make a reference to the PR that will fix the build errors.

#2933 is merged and it fixed some of the errors but there are more. If they can be fixed here we can maybe consider this as a standalone pull request.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

These code changes seem fine.

Is this the only crate that uses tokio streams?

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

One of our tests uses tokio::StreamExt:

use tokio::{stream::StreamExt, time::timeout};

This is a blocking change.

@jvff
Copy link
Contributor Author

jvff commented Nov 1, 2021

One of our tests uses tokio::StreamExt:

use tokio::{stream::StreamExt, time::timeout};

This is a blocking change.

This is unrelated to tokio-stream, and is fixed by #2920 (see commit becd6ca).

@jvff jvff mentioned this pull request Nov 1, 2021
3 tasks
teor2345
teor2345 previously approved these changes Nov 1, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This is unrelated to tokio-stream, and is fixed by #2920 (see commit becd6ca).

Thanks, that makes sense!

jvff added 3 commits November 2, 2021 00:43
In newer versions of Tokio `Interval` doesn't implement `Stream`, so the
wrapper types from `tokio-stream` have to be used instead.
In newer versions of Tokio the `Interval` type doesn't implement
`Stream`, so `tokio_stream::wrappers::IntervalStream` has to be used
instead.
In newer versions of Tokio `broadcast::Receiver` doesn't implement
`Stream`, so `tokio_stream::wrappers::BroadcastStream` instead. This
also requires changing the error type that is used.
@jvff
Copy link
Contributor Author

jvff commented Nov 2, 2021

Merged through #2994.

@jvff jvff closed this Nov 2, 2021
@jvff jvff deleted the use-tokio-stream-crate branch November 24, 2021 13:05
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