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

Better handling of chunks when decoding streaming data. #24

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

rodo-r2r
Copy link
Contributor

@rodo-r2r rodo-r2r commented Nov 2, 2020

This PR adds a test generating lots of small chunks in the encoding.
The idea is to stress this part of the code to make bugs easier to spot.

As a result three bugs were fixed:

  • Stop reading past input in SnappyDecompressor on chunk boundary.
  • Copy buffer into correct inidex of partially created scratch on chunk boundary.
  • Reset _scratchLength after use on chunk boundary.

This PR adds a test generating lots of small chunks in the encoding.
The idea is to stress this part of the code to make bugs easier to spot.

- Stop reading past input in SnappyDecompressor on chunk boundary.
- Copy buffer into correct inidex of partially created scratch
  on chunk boundary.
- Reset _scratchLength after use on chunk boundary.
@rodo-r2r rodo-r2r changed the title Better handling of chunked streaming data. Better handling of chunks when decoding streaming data. Nov 2, 2020
@rodo-r2r rodo-r2r marked this pull request as ready for review November 2, 2020 05:11
@rodo-r2r
Copy link
Contributor Author

rodo-r2r commented Nov 2, 2020

Hi, I'm evaluating this library as a replacement for our use of Snappy.NET (for use on .NET Core).
Looks good, but found a few bugs trying to get existing snappy files to load.

I'm pretty sure the build failure above isn't related to any changes I've made here.

Copy link
Owner

@brantburnett brantburnett left a comment

Choose a reason for hiding this comment

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

Thanks so much for the fixes, looks great. I may come back and add some more narrowly scoped unit tests, but looks good overall. I'll look at your other issue you filed shortly.

@brantburnett
Copy link
Owner

The build failure is a result of a problem in the GitHub action when the PR is from a fork, ignoring and merging.

@brantburnett brantburnett merged commit c9ddcf0 into brantburnett:main Nov 2, 2020
@github-actions

This comment has been minimized.

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.

None yet

2 participants