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

Require Send+Sync bounds for Allocation trait #1945

Merged
merged 1 commit into from Jun 26, 2022

Conversation

jhorstmann
Copy link
Contributor

Which issue does this PR close?

Closes #1944.

Rationale for this change

Send + Sync were previously implemented for Buffer which probably was not sound in case the underlying Allocation was not also Send + Sync.

What changes are included in this PR?

  • Allocation trait requires Send + Sync
  • Bytes implements Send + Sync which should be fine since it only contains a NonNull pointer in addition to the allocation
  • Buffer is then automatically Send + Sync
  • FFI_ArrowArray now has to implement Send + Sync. This requirement was previously hidden by the unsafe impl on Buffer, even before the introduction of the Allocation abstraction in Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

Are there any user-facing changes?

This could be a breaking change for users of the Allocation trait, but those usages would have been unsound before.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 24, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1945 (22b6017) into master (9f7b600) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1945      +/-   ##
==========================================
- Coverage   83.48%   83.47%   -0.01%     
==========================================
  Files         221      221              
  Lines       57054    57054              
==========================================
- Hits        47629    47627       -2     
- Misses       9425     9427       +2     
Impacted Files Coverage Δ
arrow/src/alloc/mod.rs 78.04% <ø> (ø)
arrow/src/buffer/immutable.rs 98.48% <ø> (ø)
arrow/src/bytes.rs 73.07% <ø> (ø)
arrow/src/ffi.rs 86.97% <ø> (ø)
parquet_derive/src/parquet_field.rs 65.75% <0.00%> (-0.46%) ⬇️

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 9f7b600...22b6017. Read the comment docs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I re-read https://doc.rust-lang.org/nomicon/send-and-sync.html and this change makes sense to me (push the Send / Sync validation to the Allocation

cc @tustvold and @viirya

+1 for cleaning up clippy lint

@tustvold tustvold merged commit 55d6073 into apache:master Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send + Sync impl for Allocation may not be sound unless Allocation is Send + Sync as well
5 participants