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

fix(recorder): don't automatically resume the response #2160

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

mastermatt
Copy link
Member

Per Node docs,

If no 'response' handler is added, then the response will be entirely discarded. However, if a 'response' event handler is added, then the data from the response object must be consumed, either by calling response.read() whenever there is a 'readable' event, or by adding a 'data' handler, or by calling the .resume() method. Until the data is consumed, the 'end' event will not fire. Also, until the data is read it will consume memory that can eventually lead to a 'process out of memory' error.

However, when the Recorder was enabled, Nock would sometimes resume the response stream after monky-patching the push methods. This doesn't align with the goal of being consistent with native Node. And has presented itself as an issue with Got because sometimes that client doesn't register the 'end' event listener fast enough.

Moreover, Nock was not consistent with the approach. It would only resume the response if .request was not provided a callback. Which meant the two following examples created different behavior.

http.request('http://example.com', res => {/*...*/})

// vs.

const req = http.request('http://example.com')
req.on('response', res => {/*...*/})

fixes: #2086

@mastermatt mastermatt requested a review from a team March 2, 2021 04:19
@mastermatt mastermatt added the bug label Mar 2, 2021
@koerbcm
Copy link

koerbcm commented Mar 2, 2021

👍

@gr2m gr2m merged commit e22d734 into nock:main Mar 2, 2021
@github-actions
Copy link

github-actions bot commented Mar 2, 2021

🎉 This PR is included in version 13.0.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mastermatt mastermatt deleted the 2086-recorder-resuming branch March 2, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nock recorder + Got + CookieJar + Set-Cookie causes Node to exit
3 participants