v14.10.0 changes behavior of existing code (got)

[benjamincburns]

• Version: v14.10.0
• Platform: All
• Subsystem: Uncertain

What steps will reproduce the bug?

got@11.6.1 hangs when running under v14.10.0. Works fine under earlier versions, including v14.9.0. See sindresorhus/got#1441 for more details.

As v14.10.0 was a minor version bump, I wouldn't expect it to break existing code.

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

got can make a request successfully

What do you see instead?

got hangs indefinitely (promise doesn't resolve - not sure why)

Additional information

got has over 12 million weekly downloads. This issue should probably be addressed quickly.

[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

[targos]

I'm bisecting...

[targos]

4bb4007 is the first bad commit
Author: Robert Nagy @ronag
Date: Tue Jun 23 23:08:14 2020 +0200

stream: simpler and faster Readable async iterator

Reimplement as an async generator instead of a custom
iterator class.

Backport-PR-URL: https://github.com/nodejs/node/pull/34887
PR-URL: https://github.com/nodejs/node/pull/34035
Refs: https://github.com/nodejs/node/issues/34680
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

[ronag]

Would someone mind to dig into this a bit a look exactly where in got the problem occurs? I'm a bit swamped today but might be able to take a look if I know more specifically where e.g. async generators come into play?

[ronag]

Should got be added to CITGM?

[targos]

Got was disabled in CITGM because there were issues with ESLint that made it always fail: nodejs/citgm#825, nodejs/citgm#795

[targos]

This happens because got treats calls to Stream#_destroy as error cases:

https://github.com/sindresorhus/got/blob/408e22aed99300afd4af49a4eb50963665d8a144/source/core/index.ts#L2666-L2691

The promise never resolves because _isAboutToError is checked to return early here:

https://github.com/sindresorhus/got/blob/1f132e88b4e9b295631637757d64307032a4e56e/source/as-promise/index.ts#L63-L65

The _destroy method is called from here:

node/lib/_stream_readable.js

Lines 1117 to 1119 in 1204400

	} finally {
	destroyImpl.destroyer(stream, null);
	}

[ronag]

Seems like a bug in got. However, we should probably try to avoid breaking in a semver minor.

As far as I can see the old implementation also did destroy but with a slightly different timing. Might be enough to wrap the destroy in a nextTick.

[ronag]

Ok, this might actually be an error in the PR. We should not call destroy before end has been emitted.

I'll try to prepare a PR tonight.

If someone wants to work on it asap something like this should fix it:

  try {
    const state = stream._readableState;
    while (true) {
      const chunk = stream.read();
      if (chunk !== null) {
        yield chunk;
      } else if (state.errored) {
        throw state.errored;
      } else if (state.endEmitted) {
        break;
      } else if (state.closed) {
        // TODO(ronag): ERR_PREMATURE_CLOSE?
        break;
      } else {
        await new Promise(next);
      }
    }
  } catch (err) {
    destroyImpl.destroyer(stream, err);
    throw err;
  } finally {
    destroyImpl.destroyer(stream, null);
  }

i.e. change state.ended to state.endEmitted.

[ronag]

I'm not sure whether the above will resolve the referenced issue though.

[richardlau]

I'm not sure whether the above will resolve the referenced issue though.

I've just tested it and it does not 😞.

Got was disabled in CITGM because there were issues with ESLint that made it always fail: nodejs/citgm#825, nodejs/citgm#795

We should unskip it in the lookup if we can. FWIW you can run "citgm got" instead of "citgm-all" to still test the module, e.g. against the current master branch: https://ci.nodejs.org/job/citgm-smoker/2461/

Edit: CI contains lint warnings similar to nodejs/citgm#795 but the test is also timed out.