-
Notifications
You must be signed in to change notification settings - Fork 821
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
Upgrade prost and tonic #898
Conversation
@@ -219,7 +221,8 @@ impl FlightService for AuthBasicProtoScenarioImpl { | |||
&self, | |||
request: Request<Streaming<FlightData>>, | |||
) -> Result<Response<Self::DoExchangeStream>, Status> { | |||
self.check_auth(request.metadata()).await?; | |||
let metadata = request.metadata(); |
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.
Without this change you get a wonderful error message which arises because the boxed decoder on Streaming
is not Sync
error: future cannot be sent between threads safely
--> integration-testing/src/flight_server_scenarios/auth_basic_proto.rs:223:59
|
223 | ) -> Result<Response<Self::DoExchangeStream>, Status> {
| ___________________________________________________________^
224 | | self.check_auth(request.metadata()).await?;
225 | | Err(Status::unimplemented("Not yet implemented"))
226 | | }
| |_____^ future created by async block is not `Send`
|
= help: the trait `Sync` is not implemented for `(dyn tonic::codec::Decoder<Item = FlightData, Error = Status> + std::marker::Send + 'static)`
note: future is not `Send` as this value is used across an await
--> integration-testing/src/flight_server_scenarios/auth_basic_proto.rs:224:9
|
224 | self.check_auth(request.metadata()).await?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ first, await occurs here, with `request` maybe used later...
note: `request` is later dropped here
--> integration-testing/src/flight_server_scenarios/auth_basic_proto.rs:224:51
|
224 | self.check_auth(request.metadata()).await?;
| ------- ^
| |
| has type `&tonic::Request<Streaming<FlightData>>` which is not `Send`
help: consider moving this into a `let` binding to create a shorter lived borrow
--> integration-testing/src/flight_server_scenarios/auth_basic_proto.rs:224:25
|
224 | self.check_auth(request.metadata()).await?;
| ^^^^^^^^^^^^^^^^^^
= note: required for the cast to the object type `dyn futures::Future<Output = std::result::Result<tonic::Response<Pin<Box<(dyn futures::Stream<Item = std::result::Result<FlightData, Status>> + Sync + std::marker::Send + 'static)>>>, Status>> + std::marker::Send`
Codecov Report
@@ Coverage Diff @@
## master #898 +/- ##
==========================================
- Coverage 82.30% 82.30% -0.01%
==========================================
Files 168 168
Lines 48031 48034 +3
==========================================
- Hits 39533 39532 -1
- Misses 8498 8502 +4
Continue to review full report at Codecov.
|
Closing as dupe of #864 |
Which issue does this PR close?
Closes #897.
Rationale for this change
See ticket
What changes are included in this PR?
Updates tonic and prost.
Are there any user-facing changes?
Tonic 0.6 contains breaking changes I think making this a breaking change by proxy. In particular tonic 0.6 changes the Sync requirements of streaming request bodies.