Repository navigation
v14.10.0 changes behavior of existing code (got) #35116
Description
Activity
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
I'm bisecting...
4bb4007 is the first bad commit
Author: Robert Nagy @ronag
Date: Tue Jun 23 23:08:14 2020 +0200stream: 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>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?
Should got be added to CITGM?
Got was disabled in CITGM because there were issues with ESLint that made it always fail: nodejs/citgm#825, nodejs/citgm#795
This happens because
gottreats calls toStream#_destroyas error cases:The promise never resolves because
_isAboutToErroris checked to return early here:The
_destroymethod is called from here:Lines 1117 to 1119 in 1204400
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.endedtostate.endEmitted.- added a commit that references this issue
on Sep 9, 2020 I'm not sure whether the above will resolve the referenced issue though.
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
masterbranch: 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.
Unfortunately it looks like
gotstill times out with #35120. It's not clear to me if this is thought to be an issue withgotitself (#35116 (comment)) or is something we need to push a new release for (cc @nodejs/streams).If we do need to push a new release we should aim to do that by tomorrow (Thursday 10th September) at the latest since:
- There are security releases due out next Tuesday and one of the vulnerabilities being addressed in v14.x is rated as critical severity. We should aim to not block people from picking up the security release if it's going to break them.
- We have a policy of not releasing on a Friday.
I can push out a new release if required. Questions:
- Is a new release required?
- If so, can the issue be fixed by a PR by tomorrow? Unfortunately stream: don't destroy readable before 'end' #35120 in its current form does not fix this issue.
- If not, should we revert 4bb4007?
cc @nodejs/releasers @nodejs/tsc
I think I've managed to figure out the problem. The old async iterator didn't destroy on success and got assumes destroy is an error.
Reacted by Richard Lau and Ben Sjoberg@targos is right. The
_isAboutToErrorcheck is done on theresponseevent after the body has been read. IfClientRequestautoDestroyset totrue, then it will hang because it assumes it's an error. I'll fix this in a few hours.Reacted by Ben SjobergSo @ronag is also right. Node.js 14.10.0 broke
autoDestroy.But since it is highly recommend that it is set toUnfortunately it has to remain astrue, I will fix this on Got side anyway.false. The fix is quite complicated and we would need to release a breaking major change.- Reacted by Myles Borins, Alex Yang and mary marchini
@richardlau should we be reverting from master as well?
it will hang because it assumes it's an error. I'll fix this in a few hours.
@szmarczak note that this is a bug on
gotside..destroy()is not an error-generating scenario,.destroy(err)is.
@MylesBorins it should be reverted in v14 and then fixed on master and v15.
Reacted by Richard Lau@joaopaulobdac would you mind providing a complete example? Are you also using got?
@szmarczak note that this is a bug on got side. .destroy() is not an error-generating scenario, .destroy(err) is.
Why is it so?
autoDestroyisfalseso it shouldn't call.destroy(), therefore_isAboutToErrorwould returnfalse. I'm fully aware that Got incorrectly throws even after the Got stream is destroyed. To useautoDestroyit needs to be a breaking major release for Got (unless I find some other way to do stuff)@joaopaulobdac That's great information! Any chance you could share a minimal reproducible example?
@szmarczak We are reverting the change. There is a problem with got but as you said we should not cause such a breaking change in semver-minor. I'm sorry for the inconvenience this has caused. Did not consider this.
That being said I'd very much appreciate if you could continue helping with a fix in v15 and making got pass in CITGM.
There is a problem with got but as you said we should not cause such a breaking change in semver-minor.
I meant that if Got was to use
autoDestroy: truethen we should do a major release at Got.I'm sorry for the inconvenience this has caused. Did not consider this.
No problem. I'm glad that we see this "bug" sooner than later :D
That being said I'd very much appreciate if you could continue helping with a fix in v15 and making got pass in CITGM.
I've been actually debugging for ~10 mins and have no thoughts to stop. I'm continuing :)
Reacted by Robert Nagy and ilnurI believe this has been resolved in latest 14.x.
Reacted by Szymon Marczak
What steps will reproduce the bug?
got@11.6.1hangs 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?
gotcan make a request successfullyWhat do you see instead?
gothangs indefinitely (promise doesn't resolve - not sure why)Additional information
gothas over 12 million weekly downloads. This issue should probably be addressed quickly.