v12.16.0 post-mortem

[targos]

Trying to document what went wrong with v12.16.0 (regressions) and what could be done in the future to reduce the risks.

ESM: Package self-resolution not behind --experimental-modules flag #31754.

• The feature initially landed with its own flag, --experimental-resolve-self (module: resolve self-references node#29327).
• The flag was then removed in module: unflag resolve self node#31002 but --experimental-conditional-exports was still required to use the feature.
• module: unflag conditional exports node#31001 removed the --experimental-conditional-exports flag.
• When #31002 and #31001 landed, ESM was already unflagged on master/v13.x.

v12.16.0 included a lot of changes related to ESM and because the unflagging is the only thing that we did not backport, we couldn't know exactly which code paths will work differently.
For future ESM backports, I suggest that we actively focus on making sure it is flagged.

V8 WASM bug #31752

Uncovered after nodejs/node#30909 landed in v12.16.0. The change was initially released in v13.5.0 but no issue was reported before we included it in v12.16.0.

Because this is a V8 bug that we were not aware of at the time of the release, I don't think we could have done better here, especially since the original change was supposed to fix a regression.

large pages change breaks the build in some cases #31249, #31520

Introduced by nodejs/node#30954.
The second issue was fixed on master in nodejs/node#31547 but isn't released yet. There seems to be no fix for the first issue at the moment.

The problem here is that none of the issues were linked back to the PR that introduced them. They do not appear in the PR's timeline.
To avoid this happening again, I think we need somehow to instruct collaborators to do these when a regression is found:

• Comment on the PR with a link to the new issue
• If the PR is not going to be reverted on master, add the backport-blocked label
• If the PR is going to be reverted on master, add the dont-land-on label to both the original and the revert PRs.

async_hooks change causes Node process to crash #31783

Introduced in nodejs/node#30965.
That bug was only reported to us after it landed in v12.16.0 and is now fixed.

http response listener throwing does not result in emit of uncaughtException #31796

Introduced in nodejs/node#30236.
That bug was only reported to us after it landed in v12.16.0 and is now fixed.

New enumerable and nonwritable field on EventEmitter caused issues #30932

Introduced in nodejs/node#30932.
That was only reported to us after it landed in v12.16.0.

It is not obvious to me what we could have done better about the last three issues. They only affect specific cases that we did not test for and were not detected by people who use or run their tests on v13.x Current.

[richardlau]

For future ESM backports, I suggest that we actively focus on making sure it is flagged.

Were these clean cherry-picks or manual backports? Should ESM changes always be manually backported due to the differences in requiring flags?

[addaleax]

Because this is a V8 bug that we were not aware of at the time of the release, I don't think we could have done better here, especially since the original change was supposed to fix a regression.

Actually … I opened https://chromium-review.googlesource.com/c/v8/v8/+/2061548 as a suggested fix for the issue, where it was pointed out that https://bugs.chromium.org/p/v8/issues/detail?id=10104 was a bug report for this that was opened a month before the problematic commit went out in 12.16.0.

I don’t know why this was reported to V8 only, but this might be a point where better communication (e.g. Node.js people being pinged by the V8 team) or us actively watching the V8 issue tracker would have helped.

To avoid this happening again, I think we need somehow to instruct collaborators to do these when a regression is found:

See also #535 for some related discussion.

Also, I’ve been thinking from time to time that it might make sense for us to have something like Refs: as a commit message field, but for PRs that the new commit fixes up (e.g. Fixes-Bug-Introduced-In: ...). Implemented properly, we could make sure that PRs that should be released together are actually released together.

http response listener throwing does not result in emit of uncaughtException

I don’t think there’s necessarily something we could have done on the process side to prevent this, but we might want to look into preventing something like this from happening again on the technical side.

InternalCallbackScope was introduced as a lower-overhead way of achieving the same thing as CallbackScope at the cost of having to do more manual control. But if we (I?) don’t manage to use it correctly, it might make sense to try to move towards using CallbackScope more.

Then there’s also the fact that this only happened because the HTTP parser has odd timing requirements around process.nextTick(). If it didn’t, we could have used the more standard (and thus more foolproof) facilities for async calls. It’s probably worth investigating why we need to special-case here in the first place, and how we can fix that.

New enumerable and nonwritable field on EventEmitter caused issues

I would personally argue that deviations from the standard JS property model (i.e. writable + configurable properties) are not something that other developers necessarily expect to handle, especially if they weren’t present before. My personal stance would be to make it policy that new properties are writable and configurable unless we have a good reason to break developers’ expectations here – what are we trying to prevent? Generally, properties are not overwritten accidentally.

[targos]

@addaleax I agree with everything you wrote.
I was trying to see if we could do better when porting changes that are already in master/current, but it's even better if the problem never lands anywhere :)

[BethGriggs]

I think this issue documents our post-mortem. We also discussed this in the Working Group meeting on 2020-02-27.

I think this can be closed now, please reopen if you disagree.