Skip to content

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

Closed
styfle wants to merge 2 commits intonodejs:mainfrom
styfle:styfle/dump-infinity
Closed

feat!: change default dump maxSize/limit to infinity#3788
styfle wants to merge 2 commits intonodejs:mainfrom
styfle:styfle/dump-infinity

Conversation

@styfle
Copy link
Copy Markdown
Member

@styfle styfle commented Oct 30, 2024 •
edited
Loading

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 mentioned this pull request Oct 30, 2024
Merged
7 tasks
@styfle styfle changed the title feat!: change default dump maxSize to infinity feat!: change default dump maxSize/limit to infinity Oct 30, 2024
ronag
ronag requested changes Oct 30, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

mcollina 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?

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
Copy Markdown
Member

ronag commented Oct 31, 2024 •
edited
Loading

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
Copy Markdown
Member

mcollina commented Oct 31, 2024

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
Copy Markdown
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

Reviewers

@ronag ronag ronag requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@styfle @mcollina @ronag