Skip to content

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

Merged
gr2m merged 2 commits intonock:mainfrom
mastermatt:2086-recorder-resuming
Mar 2, 2021
Merged

fix(recorder): don't automatically resume the response#2160
gr2m merged 2 commits intonock:mainfrom
mastermatt:2086-recorder-resuming

Conversation

@mastermatt
Copy link
Copy Markdown
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
Copy Markdown

koerbcm commented Mar 2, 2021

👍

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

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