Got: 11.6.1 hangs when running on node v14.10.0

[benjamincburns]

Describe the bug

• Node.js version: v14.10.0
• OS & version: all

I use got in a node CLI tool. I accidentally published it with a package-lock.json instead of npm-shrinkwrap.json. Unfortunately this meant that my tool was tracking the latest version of got. When [email protected] was released, my tool started hanging when making HTTP requests when running under node v14.10.0.

I also see that the CI build for got is broken for the v11.6.1 release commit. The failure occurred on the node v14 build, and apparently the build failed due to tests timing out, which suggests that this issue is already reproduced extensively by the existing test suite.

Actual behavior

got hangs indefinitely when executing this statement

Expected behavior

The line lined above would execute as normal.

Code to reproduce

See link above, or, per your CI (also linked above), just run your test suite under node v14.10.0

Checklist

• [x] I have read the documentation.
• [x] I have tried my code with the latest version of Node.js and Got.

[benjamincburns]

I did a bit of testing, and it appears that this is only an issue on node v14.10.0 (v14.9.0 and earlier work fine). Node v14.10.0 was also released earlier today. I've done no digging to identify the actual bug, but given the version-specific nature of the beast, I would imagine that this is a bug in NodeJS, as a minor release has changed behaviour in existing code.

[targos]

Reproduction:

require('got')('https://www.google.com').then(console.log, console.log)

This logs the response in Node.js 14.9.0 and exits without any logs in Node.js 14.10.0

[Giotino]

Following the discussion on https://github.com/nodejs/node/issues/35116

this[kStopReading] should be an indicator of "stop reading the stream" and it's set to true in _destroy.
But in the _isAboutToError getter this[kStopReading] is used as an indicator of an error state.

As pointed out by @targos: Node.JS v14.10.0 destroys the stream even on success, which is correct.

[ronag]

Could somebody apply the following PR on v14.x-staging, build it and try to reproduce this issue? To see if it resolves it.

[Giotino]

@ronag I see no difference.
Neither with this

require('got')('https://www.google.com').then(console.log, console.log)

Nor with the got tests.

[ronag]

This is probably a problem with Node destroying the stream even though autoDestroy: false. https://github.com/nodejs/node/pull/35122

I would in general recommend that destroy() after 'end' has been emitted should not be an error. Would also recommend autoDestroy: true in order to behave as normal streams.

[szmarczak]

https://github.com/nodejs/node/issues/35116#issuecomment-689723287

[umaar]

Heads up on v14.10.0 got wasn't working for me with this code, ended up completely blowing through my API quota, as no error is thrown. Could be an idea to have got tested against the latest current Node.js?

[wibobm]

Noticed same hang when updating to node 14.10. In my application that contains extensive tests the most recent version of GOT that appears to works with node 14.10 is v11.5.2. As of the time that I posted this the most recent version of GOT is v11.6.2.

update - GOT v11.6.2 does work with node 14.10.1.

[richardlau]

Node.js 14.10.1 was released today which reverted a commit that was introduced in 14.10.0 and should make things work again. There's ongoing work both in got and in Node.js to fix things for the next major version of Node.js (15).

[benjamincburns]

@umaar I'm not a contributor to this project, so take this with a grain of salt, but my understanding is that got is tested against the latest release. You'll see there's a link to a failing CI build in my original description of the bug. I think what happened in this case was the timing of the node 4.10.0 release was so close to the timing of the got 11.6.1 release that the failure wasn't observed in the pre-release test run, but instead it became visible when building the tag after the release had been published.

There isn't really a great way to solve for this. The publish process could get tightened up a bit, but that doesn't solve for the hypothetical case where node 14.10.0 would've been released just after got's tag built successfully. In that scenario we still would've run into this problem, though with less direct evidence of the problem's existence. In practice this is a rather rare sort of bug to crop up (node is generally very good at testing that its changes comply with semver), and the timing coinciding this closely makes it an especially rare/unlikely event. Because of this, the problem is already about as mitigated as it can be from the got project's side, and the effort involved in tightening up got's release process likely wouldn't provide any measurable additional quality assurance.

Instead, a much higher value effort would be to fix the issues that prevented got from running in node's pre-release CITGM testing, as mentioned in this comment. From what I see in that thread, this work is already being done.

[szmarczak]

Closing since it's fixed on Node.js' side.

If you have any other ideas / comments, please share them at #1535 instead.