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

Fix BufferReader.read_strings mishandling feeding from buffer #72

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

mitsuhiko
Copy link
Contributor

This solves an issue where the code would incorrectly read past the number of read characters that came from the network stream.

We are seeing various problems reading from the network stream and I suspect this is caused by the buffered reader. We identified one case already where it would definitely read bad data which this pull request fixes.

This solves an issue where the code would incorrectly read past the
number of read characters that came from the network stream.
@xzkostyan
Copy link
Member

Great!

Please fix flake8 to ensure that all tests are passed:

./tests/test_connect.py:211:70: W292 no newline at end of file

bretthoerner added a commit to getsentry/snuba that referenced this pull request Feb 14, 2019
* Use clickhouse-driver fork to fix read error.

See: mymarilyn/clickhouse-driver#72

* Add note so we're not confused later.
@mitsuhiko
Copy link
Contributor Author

@xzkostyan sorry for that, fixed.

@xzkostyan xzkostyan merged commit 479dae1 into mymarilyn:master Feb 14, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.994% when pulling 6449158 on mitsuhiko:bugfix/buffer-reader into 674996d on mymarilyn:master.

@xzkostyan
Copy link
Member

@mitsuhiko hi!

I've bumped package version to 0.0.18.

bretthoerner added a commit to getsentry/snuba that referenced this pull request Feb 19, 2019
bretthoerner added a commit to getsentry/snuba that referenced this pull request Feb 19, 2019
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.

3 participants