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

feat!: change default dump maxSize/limit to infinity #3788

Closed
wants to merge 2 commits into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Oct 30, 2024

This relates to...

Rationale

I think dump() should be able to dump the entire response body, not limited to a max size, you often don't know how big the response body will be.

Changes

N/A

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

  • Change default maxSize from 1048576 (1024 * 1024) to null (Infinity)
  • Change default limit from 131072 (128 * 1024) to null (Infinity)

Status

@styfle styfle changed the title feat!: change default dump maxSize to infinity feat!: change default dump maxSize/limit to infinity Oct 30, 2024
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

If the response is too big it can cause significant delay before the response is dumped. IMHO it's better to just close the connection in those cases.

@styfle
Copy link
Member Author

styfle commented Oct 30, 2024

@ronag Feel free to make any changes to this PR.

I created this PR since there was no discussion happening on the original docs PR and a DM to @mcollina suggested creating a PR to change the value to infinity.

@mcollina
Copy link
Member

If the response is too big it can cause significant delay before the response is dumped. IMHO it's better to just close the connection in those cases.

I agree, but then why do we have a .dump() method to begin with?

Calling .dump() with a limit is dangerous, as users would still need to consume the rest of stream or risk crashing their process.

@ronag
Copy link
Member

ronag commented Oct 31, 2024

If the response is too big it can cause significant delay before the response is dumped. IMHO it's better to just close the connection in those cases.

I agree, but then why do we have a .dump() method to begin with?

We have it so that if a user aborts a request with a small response we consume it and re-use the connection rather than killing it and starting a new one for the next request.

Calling .dump() with a limit is dangerous, as users would still need to consume the rest of stream or risk crashing their process.

I don't understand? If the limit is reached then dump will kill the connection.

@mcollina
Copy link
Member

Ah I misunderstood this. Then we can close this.

@mcollina mcollina closed this Oct 31, 2024
@styfle styfle deleted the styfle/dump-infinity branch October 31, 2024 14:58
@styfle
Copy link
Member Author

styfle commented Oct 31, 2024

If the limit is reached then dump will kill the connection.

To be clear, if the connection is killed then there is no risk of memory leak or process crash?

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