Skip to content

Conversation

thrau
Copy link
Contributor

@thrau thrau commented Dec 20, 2022

This PR adds a test and fix for LimitedStream to work with raw IO streams. Raw IO streams can return fewer bytes than passed to size in a read call, and previously LimitedStream.read was not equipped to handle these cases.
I had to

  • add a method to deal with cases where LimitedStream.read(-1) is called, that would exhaust the remainder of the stream and actually return the result. It uses a common "read into buffer" pattern.
  • change the condition on what the read method considers as a client disconnect. The added tests should clarify further what is considered a disconnect and what isn't.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@thrau
Copy link
Contributor Author

thrau commented Jan 5, 2023

could i get your sentiment whether this is in principle an acceptable solution @davidism? happy to then complete the other PR tasks and investigate the failing checks.

@davidism davidism added this to the 2.2.3 milestone Feb 6, 2023
@davidism davidism force-pushed the limitedstream-raw-io-support branch from d6b7b73 to f5e1b02 Compare February 6, 2023 16:11
@davidism davidism changed the base branch from main to 2.2.x February 6, 2023 16:11
@davidism
Copy link
Member

davidism commented Feb 6, 2023

Refactored this to fix exhaust as well and use the same logic for both. Previously, exhaust was assuming that read would return the size that was requested. Also added a changelog and comments to the code and test.

ensure exhaust accounts for returned size, not requested size
add comments about exhaust/disconnect logic
clean up test
@davidism davidism force-pushed the limitedstream-raw-io-support branch from f5e1b02 to 591b115 Compare February 6, 2023 16:16
@davidism davidism merged commit 64f0eb4 into pallets:2.2.x Feb 6, 2023
@thrau
Copy link
Contributor Author

thrau commented Feb 6, 2023

thanks a lot for pushing it over the line @davidism 🙏

@thrau thrau deleted the limitedstream-raw-io-support branch February 6, 2023 16:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LimitedStream not respecting read contract of RawIOBase.
2 participants