Skip to content

Conversation

@tustvold
Copy link
Contributor

@tustvold tustvold commented Nov 1, 2021

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.

@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Nov 1, 2021
request: Request<Streaming<FlightData>>,
) -> Result<Response<Self::DoExchangeStream>, Status> {
self.check_auth(request.metadata()).await?;
let metadata = request.metadata();
Copy link
Contributor Author

@tustvold tustvold Nov 1, 2021

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-commenter
Copy link

Codecov Report

Merging #898 (83b2b60) into master (06f730e) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 83b2b60 differs from pull request most recent head 9b5ea1e. Consider uploading reports for the commit 9b5ea1e to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
arrow-flight/src/arrow.flight.protocol.rs 0.00% <ø> (ø)
...ng/src/flight_server_scenarios/auth_basic_proto.rs 0.00% <0.00%> (ø)
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (-0.46%) ⬇️
arrow/src/datatypes/datatype.rs 64.93% <0.00%> (-0.44%) ⬇️
arrow/src/array/transform/mod.rs 85.33% <0.00%> (+0.13%) ⬆️
parquet/src/encodings/encoding.rs 93.71% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06f730e...9b5ea1e. Read the comment docs.

@tustvold
Copy link
Contributor Author

tustvold commented Nov 1, 2021

Closing as dupe of #864

@tustvold tustvold closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow-flight Changes to the arrow-flight crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update prost and tonic

2 participants