Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v12.16.1 proposal #31781

Closed
wants to merge 12 commits into from
Closed

v12.16.1 proposal #31781

wants to merge 12 commits into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Feb 13, 2020

2020-02-18, Version 12.16.1 'Erbium' (LTS), @MylesBorins

Notable changes

Node.js 12.16.0 included 6 regressions that are being fixed in this release

Accidental Unflagging of Self Resolving Modules:

12.16.0 included a large update to the ESM implementation. One of the new features,
Self Referential Modules, was accidentally released without requiring the --experimental-modules
flag. This release is being made to appropriately flag the feature.

Process Cleanup Changed Introduced WASM-Related Assertion:

A change during Node.js process cleanup led to a crash in combination with
specific usage of WASM. This has been fixed by partially reverted said change.
A regression test and a full fix are being worked on and will likely be included
in future 12.x and 13.x releases.

Use Largepages Runtime Option Introduced Linking Failure:

A Semver-Minor change to introduce --use-largepages as a runtime option
introduced a linking failure. This had been fixed in master but regressed as the fix has not yet gone out
in a Current release. The feature has been reverted, but will be able to reland with a fix in a future
Semver-Minor release.

Async Hooks was Causing an Exception When Handling Errors:

Changes in async hooks internals introduced a case where an internal api call could be called with undefined
causing a process to crash. The change to async hooks was reverted. A regression test and fix has been proposed
and the change could re-land in a future Semver-Patch release if the regression is reliably fixed.

New Enumerable Read-Only Property on EventEmitter breaks @types/extend

A new property for enumerating events was added to the EventEmitter class. This
broke existing code that was using the @types/extend module for extending classses
as @types/extend was attemping to write over the existing field which the new
change made read-only. As this is the first property on EventEmitter that is
read-only this feature could be considered Semver-Major. The new feature has been
reverted but could re-land in a future Semver-Minor release if a non breaking way
of applying it is found.

Exceptions in the HTTP parser were not emitting an uncaughtException

A refactoring to Node.js interanls resulted in a bug where errors in the HTTP
parser were not being emitted by process.on('uncaughtException') when the async_hooks after
hook exists. The fix to this bug has been included in this release.

Commits

  • [51fdd759b9] - async_hooks: ensure event after been emitted on runInAsyncScope (legendecas) #31784
  • [7a1b0ac06f] - Revert "build: re-introduce --use-largepages as no-op" (Myles Borins) #31782
  • [a53eeca2a9] - Revert "build: switch realpath to pwd" (Myles Borins) #31782
  • [6d432994e6] - Revert "build: warn upon --use-largepages config option" (Myles Borins) #31782
  • [a5bc00af12] - Revert "events: allow monitoring error events" (Myles Borins)
  • [f0b2d875d9] - module: 12.x self resolve flag as experimental modules (Guy Bedford) #31757
  • [42b68a4e24] - src: inform callback scopes about exceptions in HTTP parser (Anna Henningsen) #31801
  • [065a32f064] - Revert "src: make --use-largepages a runtime option" (Myles Borins) #31782
  • [3d5beebc62] - Revert "src: make large_pages node.cc include conditional" (Myles Borins) #31782
  • [43d02e20e0] - src: keep main-thread Isolate attached to platform during Dispose (Anna Henningsen) #31795
  • [7a5954ef26] - src: fix -Winconsistent-missing-override warning (Colin Ihrig) #30549

cjihrig and others added 2 commits February 13, 2020 08:42
This commit addresses the following warning:

../src/node_api.cc:28:19: warning: 'mark_arraybuffer_as_untransferable'
overrides a member function but is not marked 'override'
      [-Winconsistent-missing-override]
  v8::Maybe<bool> mark_arraybuffer_as_untransferable(
                  ^
../src/js_native_api_v8.h:42:27: note: overridden virtual function is here
  virtual v8::Maybe<bool> mark_arraybuffer_as_untransferable(

PR-URL: #30549
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
PR-URL: #31757
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. meta Issues and PRs related to the general management of the project. node-api Issues and PRs related to the Node-API. v12.x labels Feb 13, 2020
@MylesBorins MylesBorins requested review from a team February 13, 2020 23:11
MylesBorins added a commit that referenced this pull request Feb 13, 2020
Notable changes:

Node.js 12.16.0 included large update to the ESM implementation in the
12.x release stream. One of the new features, Self Referential Modules,
accidentally was released not behind the `--experimental-modules` flag.
This release is being made specifically to appropraitely flag the
feature which is not yet ready to be release on LTS.

PR-URL: #31781
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 13, 2020

@MylesBorins
Copy link
Contributor Author

I've kicked off CI + CITGM. I might push this before CITGM finishes, depending on how long it takes as the changes in this release should have 0 ecosystem. breakage that isn't expected / desired (only moves a feature behind a flag).

If anyone knows of other 12.x regressions I'd be happy to bring them in here.

@richardlau
Copy link
Member

If anyone knows of other 12.x regressions I'd be happy to bring them in here.

There were a couple of other regressions introduced in 12.16.0 too:

@MylesBorins
Copy link
Contributor Author

@richardlau PTAL #31782

If those get signed off I'll roll them in and hopefully get this release out before EOD

@MylesBorins
Copy link
Contributor Author

Taking a bit of time to get the test suite done for the linux containered env. Might have to punt this release to tomorrow. Any concerns?

MylesBorins added a commit that referenced this pull request Feb 14, 2020
Notable changes:

Node.js 12.16.0 included 3 regressions that are being fixed in this release

Accidental Unflagging of Self Resolving Modules:

12.16.0 included a large update to the ESM implementation. One of
the new features, Self Referential Modules, was accidentally released
without requiring the `--experimental-modules` flag. This release is
being made to appropriately flag the feature.

Isolate Disposal Order Change Introduced WASM Assertion:

A change in the order of Isolate Disposal created an assertion
error in WASM usage in V8. This was not present on 13.x or higher as
they use a newer version of V8. The change was reverted. A
regression test and fix are being worked on and will likely be
included in a future 12.x release

Use Largepages Runtime Option Introduced Linking Failure:

A Semver-Minor change to introduce `--use-largepages` as a runtime
option introduced a linking failure. This had been fixed in master
but regressed as the fix has not yet gone out in a Current release.
The feature has been reverted, but will be able to re-land with a fix
in a future Semver-Minor release.

Async Hooks was Causing an Exception When Handling Errors:

Changes in async hooks internals introduced a case where an internal
api call could be called with undefined causing a process to crash.
The change to async hooks was reverted. A regression test and fix has
been proposed and the change could re land in a future Semver-Patch
release if the regression is reliably fixed.

PR-URL: #31781
@MylesBorins MylesBorins requested a review from Trott February 14, 2020 08:17
MylesBorins added a commit that referenced this pull request Feb 18, 2020
Notable changes:

Node.js 12.16.0 included 6 regressions that are being fixed in this
release

**Accidental Unflagging of Self Resolving Modules**:

12.16.0 included a large update to the ESM implementation. One of the
new features, Self Referential Modules, was accidentally released
without requiring the `--experimental-modules` flag. This release is
being made to appropriately flag the feature.

**Process Cleanup Changed Introduced WASM-Related Assertion**:

A change during Node.js process cleanup led to a crash in combination
with specific usage of WASM. This has been fixed by partially reverted
said change. A regression test and a full fix are being worked on and
will likely be included in future 12.x and 13.x releases.

**Use Largepages Runtime Option Introduced Linking Failure**:

A Semver-Minor change to introduce `--use-largepages` as a runtime
option introduced a linking failure. This had been fixed in master but
regressed as the fix has not yet gone out in a Current release. The
feature has been reverted, but will be able to reland with a fix in a
future Semver-Minor release.

**Async Hooks was Causing an Exception When Handling Errors**:

Changes in async hooks internals introduced a case where an internal
api call could be called with undefined causing a process to crash. The
change to async hooks was reverted. A regression test and fix has been
proposed and the change could re land in a future Semver-Patch release
if the regression is reliably fixed.

**New Enumerable Read-Only Property on EventEmitter breaks @types/extend**

A new property for enumerating events was added to the EventEmitter
class. This broke existing code that was using the `@types/extend`
module for extending classses as `@types/extend` was attemping to write
over the existing field which the new change made read-only. As this is
the first property on EventEmitter that is read-only this feature could
be considered Semver-Major. The new feature has been reverted but could
re land in a future Semver-Minor release if a non breaking way of
applying it is found.

**Exceptions in the HTTP parser were not emitting an uncaughtException**

A refactoring to Node.js interanls resulted in a bug where errors in
the HTTP parser were not being emitted by
`process.on('uncaughtException')`. The fix to this bug has been
included in this release.

PR-URL: #31781
@MylesBorins
Copy link
Contributor Author

Hey all. I've done another pass on this. There are 4 changes that have been reverted and 2 that have had patches landed to fix the bugs that were introduced. I did my best to document what happened in the notable changes section as well as how these changes can be re-landed.

We can still make other changes to this before we push it out tomorrow, but I could really use support here as I have limited time to actually do the release.

Apologies in advance for the pace of these reversions and how this release was handled, doing my best to handle in a timely fashion while traveling and being off (today is a vacation day for US).

As mentioned above a broader post-mortem here would be very useful including discussion on how we handle regressions. We should likely schedule this for the next release WG meeting

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 18, 2020

doc/changelogs/CHANGELOG_V12.md Outdated Show resolved Hide resolved
doc/changelogs/CHANGELOG_V12.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

Fwiw, I’ve opened an issue around how we handle regressions in LTS releases in the future: nodejs/Release#535

I also agree that it makes sense to try to put together a post-mortem for the regressions introduced here once this release is out.

Notable changes:

Node.js 12.16.0 included 6 regressions that are being fixed in this
release

Accidental Unflagging of Self Resolving Modules:

12.16.0 included a large update to the ESM implementation. One of the
new features, Self Referential Modules, was accidentally released
without requiring the `--experimental-modules` flag. This release is
being made to appropriately flag the feature.

Process Cleanup Changed Introduced WASM-Related Assertion:

A change during Node.js process cleanup led to a crash in combination
with specific usage of WASM. This has been fixed by partially reverted
said change. A regression test and a full fix are being worked on and
will likely be included in future 12.x and 13.x releases.

Use Largepages Runtime Option Introduced Linking Failure:

A Semver-Minor change to introduce `--use-largepages` as a runtime
option introduced a linking failure. This had been fixed in master but
regressed as the fix has not yet gone out in a Current release. The
feature has been reverted, but will be able to reland with a fix in a
future Semver-Minor release.

Async Hooks was Causing an Exception When Handling Errors:

Changes in async hooks internals introduced a case where an internal
api call could be called with undefined causing a process to crash. The
change to async hooks was reverted. A regression test and fix has been
proposed and the change could re-land in a future Semver-Patch release
if the regression is reliably fixed.

New Enumerable Read-Only Property on EventEmitter breaks @types/extend:

A new property for enumerating events was added to the EventEmitter
class. This broke existing code that was using the `@types/extend`
module for extending classses as `@types/extend` was attemping to write
over the existing field which the new change made read-only. As this is
the first property on EventEmitter that is read-only this feature could
be considered Semver-Major. The new feature has been reverted but could
re-land in a future Semver-Minor release if a non breaking way of
applying it is found.

Exceptions in the HTTP parser were not emitting an uncaughtException:

A refactoring to Node.js interanls resulted in a bug where errors in
the HTTP parser were not being emitted by
process.on('uncaughtException') when the `async_hooks` `after` hook
exists. The fix to this bug has been included in this release.

PR-URL: #31781
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 18, 2020

MylesBorins added a commit that referenced this pull request Feb 18, 2020
MylesBorins added a commit that referenced this pull request Feb 18, 2020
Notable changes:

Node.js 12.16.0 included 6 regressions that are being fixed in this
release

**Accidental Unflagging of Self Resolving Modules**:

12.16.0 included a large update to the ESM implementation. One of the
new features, Self Referential Modules, was accidentally released
without requiring the `--experimental-modules` flag. This release is
being made to appropriately flag the feature.

**Process Cleanup Changed Introduced WASM-Related Assertion**:

A change during Node.js process cleanup led to a crash in combination
with specific usage of WASM. This has been fixed by partially reverted
said change. A regression test and a full fix are being worked on and
will likely be included in future 12.x and 13.x releases.

**Use Largepages Runtime Option Introduced Linking Failure**:

A Semver-Minor change to introduce `--use-largepages` as a runtime
option introduced a linking failure. This had been fixed in master but
regressed as the fix has not yet gone out in a Current release. The
feature has been reverted, but will be able to reland with a fix in a
future Semver-Minor release.

**Async Hooks was Causing an Exception When Handling Errors**:

Changes in async hooks internals introduced a case where an internal
api call could be called with undefined causing a process to crash. The
change to async hooks was reverted. A regression test and fix has been
proposed and the change could re land in a future Semver-Patch release
if the regression is reliably fixed.

**New Enumerable Read-Only Property on EventEmitter breaks @types/extend**

A new property for enumerating events was added to the EventEmitter
class. This broke existing code that was using the `@types/extend`
module for extending classses as `@types/extend` was attemping to write
over the existing field which the new change made read-only. As this is
the first property on EventEmitter that is read-only this feature could
be considered Semver-Major. The new feature has been reverted but could
re land in a future Semver-Minor release if a non breaking way of
applying it is found.

**Exceptions in the HTTP parser were not emitting an uncaughtException**

A refactoring to Node.js interanls resulted in a bug where errors in
the HTTP parser were not being emitted by
`process.on('uncaughtException')`. The fix to this bug has been
included in this release.

PR-URL: #31781
MylesBorins added a commit to nodejs/nodejs.org that referenced this pull request Feb 18, 2020
@MylesBorins
Copy link
Contributor Author

landed in d3fb168

@devsnek devsnek deleted the v12.16.1-proposal branch February 19, 2020 07:55
@codebytere codebytere mentioned this pull request Mar 24, 2020
@targos targos added release Issues and PRs related to Node.js releases. and removed c++ Issues and PRs that require attention from people who are familiar with C++. meta Issues and PRs related to the general management of the project. node-api Issues and PRs related to the Node-API. labels Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Issues and PRs related to Node.js releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.