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

Support infinite responses in bodyWrapper #342

Merged
merged 15 commits into from
Apr 6, 2023

Conversation

chunweii
Copy link
Contributor

Fixes #245.

@chunweii chunweii marked this pull request as draft March 22, 2023 07:56
@coveralls
Copy link
Collaborator

coveralls commented Mar 22, 2023

Coverage Status

Coverage: 95.678% (+0.8%) from 94.828% when pulling 2b1b1d6 on chunweii:infinite-bodywrapper into 1e30c27 on gavv:master.

@chunweii chunweii marked this pull request as ready for review March 22, 2023 12:40
@gavv gavv added the ready for review Pull request can be reviewed label Mar 22, 2023
Copy link
Owner

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for PR! Here are some comments.

body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Outdated Show resolved Hide resolved
body_wrapper.go Show resolved Hide resolved
@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Mar 29, 2023
@chunweii chunweii requested a review from gavv April 1, 2023 10:59
@gavv gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Apr 1, 2023
@gavv gavv merged commit 4f721e8 into gavv:master Apr 6, 2023
@gavv gavv removed the ready for review Pull request can be reviewed label Apr 6, 2023
@gavv
Copy link
Owner

gavv commented Apr 6, 2023

Thanks a lot for PR!

Big work and I think the code is correct.

bodyWrapper was an intricate piece from the beginning...

I pushed a follow-up commit with an attempt to refactor it a bit: 73a6521

What I changed:

  • added comments

  • renamed "origReader" to "httpReader" and "cancelFunc" to "httpCancelFunc", to make it clear that they correspond to original HTTP response

  • renamed "currReader" to "memReader" and "origBytes" to "memBytes", to make it clear that they correspond to in-memory storage

  • refactored Read: now it explicitly states three cases: rewinds are disabled, reading from HTTP; rewinds are enabled, reading from HTTP; reading from memory

  • when Read is reading from memory, it now returns cached error only after reading the whole body, for more strict emulation of original response

  • renamed "readNext" to "httpReadNext" and "readFull" to "httpReadFull", to make it clear that they read from HTTP

  • Rewind now resets reader only when it makes sense

  • in httpReadNext, we now correctly handle the case when when error is returned, but n > 0; in this case we should still append bytes to memory

  • in httpReadFull is refactored a bit

  • I renamed TestBodyWrapper_InfiniteResponses to TestBodyWrapper_Memory, because it does not actually test infinite responses, but instead tests how response is stored into memory in various cases

  • I reworked TestBodyWrapper_PartialRead so that it actually requests more bytes than returned (request 10, got 4), otherwise reads were not really partial

@gavv
Copy link
Owner

gavv commented Apr 6, 2023

Another follow-up commit: 924c915 - improvements and refactoring of body wrapper tests.

@gavv
Copy link
Owner

gavv commented Apr 6, 2023

A few more follow-up commits:

In addition, I created a few new issues for expanding tests even further:

@gavv
Copy link
Owner

gavv commented Apr 6, 2023

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.

Support infinite responses in bodyWrapper
3 participants