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

Async GzDecoder #185

Merged
merged 12 commits into from
Mar 13, 2019
Merged

Async GzDecoder #185

merged 12 commits into from
Mar 13, 2019

Conversation

quininer
Copy link
Contributor

@quininer quininer commented Feb 14, 2019

This PR implements async gzip header parsing. It will fixed #179 .

  • It changed some behavior of GzDecoder and was a breaking change.

src/gz/mod.rs Outdated Show resolved Hide resolved
@quininer quininer changed the title [WIP] Async GzDecoder Async GzDecoder Feb 15, 2019
@alexcrichton
Copy link
Member

Thanks for the PR! I've done a quick skim over this and was wondering if a few high-level points could be knocked out before doing a deeper review:

  • Instead of having an enum of states we cycle between, could we just buffer data internally until the whole header has been read? In theory the header is quite small and won't take long, even over async connections, to wait for it to all arrive.
  • Could the new2 functions be avoided? Ideally this doesn't affect the public API, only adding a few trait impls as necessary to publicly exposed types.

@quininer
Copy link
Contributor Author

  • I have tried to use BufRead instead of a state machine for parsing, but GzWriter doesn't provide BufRead for this, so I switched to a state machine instead. If you prefer BufRead, I am willing to make changes.
  • In an async context, we usually don't want to consume read before polled. This caused us to not get the header immediately, which is a breaking change.

@alexcrichton
Copy link
Member

Right yeah I don't think we need to deal with BufRead, just read data into a local buffer until the full header is available and then we parse it all in one go.

I'm not sure I understand your comments about new2 though, can you clarify?

@quininer
Copy link
Contributor Author

I may have thought wrong before. :(

@quininer
Copy link
Contributor Author

quininer commented Mar 7, 2019

ping? @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Oh oops sorry for the delay!

src/gz/bufread.rs Show resolved Hide resolved
src/gz/bufread.rs Outdated Show resolved Hide resolved
src/gz/bufread.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member

Ok looks good to me, thanks!

@alexcrichton alexcrichton merged commit 433f50c into rust-lang:master Mar 13, 2019
@quininer
Copy link
Contributor Author

Can you release a new version for it?

@alexcrichton
Copy link
Member

Published!

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.

missing AsyncRead + AsyncWrite for gzip implementations
2 participants