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

Update bytes to v0.6 #2323

Closed
wants to merge 3 commits into from
Closed

Update bytes to v0.6 #2323

wants to merge 3 commits into from

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented Nov 7, 2020

@Urhengulas
Copy link
Contributor

This doesn't currently compile due to the tokio::ReadBuf API.

@olix0r: Thank you for taking ownership for this :) Can you please elaborate what's holding the PR back?

@olix0r
Copy link
Contributor Author

olix0r commented Nov 10, 2020

@Urhengulas Sorry, that comment is outdated (addressed in 34e60f7). It looks like there are some remaining changes needed in examples that I can try to fix. I won't be offended if someone else takes this branch and gets it over the line, though ;)

@olix0r
Copy link
Contributor Author

olix0r commented Nov 10, 2020

Also, this is blocked on a few other PRs:

@paolobarbolini
Copy link
Contributor

My h2 PR does pass CI hyperium/h2#497

@olix0r
Copy link
Contributor Author

olix0r commented Nov 10, 2020

@paolobarbolini whoops! didn't mean to double up on those PRs, I just didn't look before branching. Closed my open PRs in favor of yours.

@paolobarbolini
Copy link
Contributor

It looks like there are some remaining changes needed in examples that I can try to fix

It probably just needs bytes::buf::BufExt to be changed into bytes::Buf

@olix0r
Copy link
Contributor Author

olix0r commented Nov 10, 2020

OK, build passes. gitcop won't tell me what it doesn't like(?!), so i'm going to rebase and force push.

This changes updates the `bytes` dependency to v0.6.0.
@@ -188,7 +189,9 @@ where
if self.read_buf_remaining_mut() < next {
self.read_buf.reserve(next);
}
let mut buf = ReadBuf::uninit(&mut self.read_buf.bytes_mut()[..]);
let dst = self.read_buf.bytes_mut();
let dst = unsafe { &mut *(dst as *mut _ as *mut [MaybeUninit<u8>]) };
Copy link
Contributor

Choose a reason for hiding this comment

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

That line looks threatening..

Can you please add a comment saying what it is doing?

Copy link
Contributor Author

@olix0r olix0r Nov 10, 2020

Choose a reason for hiding this comment

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

I don't actually know what it's doing ;)

I borrowed this from https://github.com/tokio-rs/tokio/blob/d78655337a68bded305782a8a8b4ac7be42aa6a7/tokio/src/io/util/read_buf.rs#L53-L55, which was the only updated use of ReadBuf::uninit I could find.

I would appreciate feedback from someone who understands these APIs better than I do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But let's see who introduced it 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added a comment summarizing that discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect 😄

@seanmonstar
Copy link
Member

Don't worry about gitcop (it's not "required"), it's just a reminder to me to fix the commit message format.

Comment on lines +192 to 197
let dst = self.read_buf.bytes_mut();
// Safety: `dst` may include uninitialized memory, but `ReadBuf` will
// never uninitialize memory that has already been initialized.
let dst = unsafe { &mut *(dst as *mut _ as *mut [MaybeUninit<u8>]) };
let mut buf = ReadBuf::uninit(dst);
match Pin::new(&mut self.io).poll_read(cx, &mut buf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative to all of this (which avoids the scary-looking pointer cast that @Urhengulas was concerned about) is to just use tokio_util::io::poll_read_buf from the latest tokio-util here:

Suggested change
let dst = self.read_buf.bytes_mut();
// Safety: `dst` may include uninitialized memory, but `ReadBuf` will
// never uninitialize memory that has already been initialized.
let dst = unsafe { &mut *(dst as *mut _ as *mut [MaybeUninit<u8>]) };
let mut buf = ReadBuf::uninit(dst);
match Pin::new(&mut self.io).poll_read(cx, &mut buf) {
match tokio_util::io::poll_read_buf(Pin::new(&mut self.io), cx, &mut self.read_buf) {

That would mean adding a tokio-util dependency, but it's already a transitive dependency via h2.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a good solution!

And this is a useful function which can't be found in the docs... (code: https://github.com/tokio-rs/tokio/blob/master/tokio-util/src/lib.rs#L67-L134).

@hawkw
Copy link
Contributor

hawkw commented Nov 20, 2020

I believe #2339 closes this.

@olix0r olix0r closed this Nov 20, 2020
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.

5 participants