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

Lazy reading of response body #244

Closed
gavv opened this issue Jan 27, 2023 · 2 comments · Fixed by #266
Closed

Lazy reading of response body #244

gavv opened this issue Jan 27, 2023 · 2 comments · Fixed by #266
Assignees
Labels
feature New feature or request help wanted Contributions are welcome important Important task
Milestone

Comments

@gavv
Copy link
Owner

gavv commented Jan 27, 2023

Currently, response body is read entirely and closed inside Response constructor. This prevents Response to work with stream responses, e.g. SSE.

To fix this, Response should not read entire body in constructor, but instead read it only on demand when it's needed first time, e.g. when the user invoked Text() or JSON(). This way it'll continue to work as before for regular responses and wont break with stream responses, given that the user does not try to invoke inappropriate methods on them.

We need to find all places when r.content is used in response.go and replace it with a function call which will check if response body is already read, and read it if necessary.

We should add unit test for Response that checks that body is not read until a call to Text/JSON/etc.

@gavv gavv added feature New feature or request help wanted Contributions are welcome important Important task labels Jan 27, 2023
@chunweii
Copy link
Contributor

Hi @gavv, may I work on this?

@gavv
Copy link
Owner Author

gavv commented Jan 29, 2023

Sure, thanks!

@gavv gavv closed this as completed in #266 Feb 8, 2023
@gavv gavv added this to the v2 milestone Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Contributions are welcome important Important task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants