Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Bytes::from_owner #742
feat: Bytes::from_owner #742
Changes from 11 commits
fad291f
905b6af
e7ccb31
713e09c
811c11c
ddf5f28
dc98465
bd89523
d419b2b
74f138b
d3b6798
8f1ddeb
1d7ef85
6271eff
8e881a8
d1aed1d
ddcb7ce
d6b35ae
d170ece
01b9792
9b4e1bc
348b822
223d621
63d2589
020a585
e10cfb6
0173061
12de708
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Doesn't this need some kind of StableDeref bound to protect against weird edge cases where the referenced region changes over time?
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.
The
string
crate had a similar issue: carllerche/string#9I do wonder if
Sync
would be sufficient here... In general, we probably do need aSend
bound as well sinceBytes
isSend
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.
Sync wouldn't cover it since you can have a thread-safe implementation that still mutates internally.
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.
As for
StableDeref
isn't callingas_ref
just once, never moving the memory afterwards because it'sBox
ed and never calling any other method already guaranteeing theBytes
invariants and soundness in general?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.
Hmm yeah only calling as_ref once may actually make this fine.
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.
There's a safe way for it to still be UB, even with only calling it once. If I pass in a form of
Arc<Mutex<_>>
, and then afterwards change the internal buffer completely (maybe a reallocation), the captured pointer could be referring to invalid memory.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.
The reference we get back from the as_ref needs to be valid for the remaining lifetime of the object though - it can't be swapped out without a call to an &mut method that would ensure the initial borrow is gone.
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.
No, it's sound as-is. You cannot write a safe thing where this breaks even with
Arc
/Mutex
because if the buffer is inside the mutex, the implementer ofAsRef
can't return the buffer fromas_ref
since it borrows from the mutex guard.As written, the
as_ref
call essentially borrows theowner
field until we run the destructor. As long as we never make any mutable function calls onowner
during that time, it's okay.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.
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.
If I'm not mistaken the current API should guard against
Owner
exposing a thread local as well.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.
I wonder if we may want our own trait instead of
AsRef<[u8]>
so we can add more methods in the future.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.
<opinionated>
A custom trait is how I had originally started implementing this, but gave up early on for a few reasons:
AsRef<[u8]>
, soBytes::from_owner(mmap);
is easy and straight-forward.Bytes::from_custom(MyMmapWrapper::new(mmap))
.AsRef<[u8]>
can be called during construction and only once. It's a trick that makes thefrom_owner
API safe.trait CustomOwner { fn try_into_mut(self) -> Result<impl CustomOwnerMut, Self>; }
(unless I'm wrong) would make it into anunsafe trait
.BytesMut
.The implementor of a trait that that exposed this would need to be very careful here to respect the
Vec<u8>
semantics. Context: IfBytes
is not unique, the existing code creates a newly allocatedVec<u8>
copy of the referenced memory. Unless due care is taken - in the specific case of anMmap
- this would likely lead an implementor to erroneously assume that they should create anMmapMut
. This would be wrong, especially if the mmapped section is backed by a file as the resulting behaviour would sometimes update the contents of a file and sometimes not, depending onBytes
's internal refcount..as_ref()
rendering that initial call unsafe too, since there's no borrow checker here to police usage.fn from_custom <T: CustomOwner>
and as a good way of exposing more of the vtable functionality, but it is not where the current pain point in the API is and there are many hurdles to get past that as has been shown in previous efforts to tackle meta: ExposeBytes
vtable #437.The full-fat custom use case would mostly cover - as far as I can tell - mixing
Bytes
created and managed via multiple custom memory allocators (QuestDB would certainly care!), but I suspect there's a more elegant way to handle those anyway.</opinionated>
.. that said, if you guys prefer that route I'm happy to amend my PR in that direction.