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

src: migrated to new V8 ArrayBuffer API #30782

Closed
wants to merge 1 commit into from

Conversation

thangktran
Copy link
Contributor

ArrayBuffer without BackingStore will soon be deprecated.

Fixes:#30529

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 3, 2019
@Trott
Copy link
Member

Trott commented Dec 4, 2019

Micro-nit for OP or whoever lands this: s/migrated/migrate/ in the commit message first line. (We're not so great at enforcing it, but our commit guidelines say to make the first word after the subsystem an imperative verb--basically, present tense.)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely LGTM, nice work!

src/aliased_buffer.h Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Show resolved Hide resolved
buf.base,
buf.len,
v8::ArrayBufferCreationMode::kInternalized);
std::move(backing));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So… This looks correct and I currently don’t have any better ideas either, but maybe to give an idea about the “big picture” here:

The idea behind AllocatedBuffer is to have something that gets turned into an ArrayBuffer in the most direct way possible. In that sense, it is basically our own version of BackingStore – a wrapper class for memory tracked through the current ArrayBuffer::Allocator. A “nicer” way to fix this would be to e.g. store the data as the std::shared_ptr<BackingStore> directly instead of using a uv_buf_t. However, I think that doesn’t work currently, because we’d have no way to implement e.g. .release() for that kind of scenario.

It might be nice if we had a way to avoid free callbacks that only run v8::ArrayBuffer::Allocator::Free() like this one. That would require an V8 API change, though, and I’m not sure what the best way for that would be.

Or, alternatively, it would be nice if we could implement AllocatedBuffer in terms of std::shared_ptr<BackingStore> as mentioned above; in that case, we’d have to figure out something clever for AllocatedBuffer::release().

(To be clear, this isn’t a request to change the code here. It’s just me rambling and trying to give some context.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think of something and prepare a PR for this.

src/js_native_api_v8.cc Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
test/addons/worker-buffer-callback/binding.cc Outdated Show resolved Hide resolved
@thangktran thangktran force-pushed the thangktran/issues/30529 branch 2 times, most recently from c7ae8d6 to 759fcdd Compare December 4, 2019 18:34
@thangktran
Copy link
Contributor Author

Micro-nit for OP or whoever lands this: s/migrated/migrate/ in the commit message first line. (We're not so great at enforcing it, but our commit guidelines say to make the first word after the subsystem an imperative verb--basically, present tense.)

fixed

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails make lint-js. Can you fix it up?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, the linter issues are probably a different thing that got fixed already. Sorry for the noise.

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Dec 6, 2019

Does the number of crashing tests in CI with this change seems to suggest there's a problem with it that hasn't been identified yet?

@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

@Trott Yes, definitely. The failing check is here:

CHECK(result.second);

It’s odd that it’s failing inconsistently across multiple tests, though.

src/node_worker.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

My guess would be that the cause is that Http2Session instances may be allocated in the same location as previous instances of that class, before the ArrayBuffer objects associated with those previous instances are garbage collected.

In that case the previous BackingStores would also still exist and refer to now-deallocated memory, and V8 refuses to create BackingStores for the new ones. That’s actually a fair complaint by V8, in a way.

I think a possible solution would be to call ArrayBuffer::Detach() from the Http2Session destructor. That should also keep us from accidentally introducing crashes if we happen to mess up in JS land.

@codecov-io
Copy link

codecov-io commented Dec 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d7b8ae7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #30782   +/-   ##
=========================================
  Coverage          ?   97.28%           
=========================================
  Files             ?      187           
  Lines             ?    62462           
  Branches          ?        0           
=========================================
  Hits              ?    60767           
  Misses            ?     1695           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b8ae7...850a80b. Read the comment docs.

@thangktran
Copy link
Contributor Author

ping for review and CI.

@targos
Copy link
Member

targos commented Dec 6, 2019

Do the changes in test/js-native-api mean that there is a beraking change for N-API ?

->Externalize(array_buffer_to_detach->GetBackingStore());
array_buffer_to_detach->Detach();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh … I think detaching the stream_buf_ab_ would invalidate stream chunks emitted to JS? Can you explain these changes here?

Copy link
Contributor Author

@thangktran thangktran Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. so stream_buf_ab_ is the only ArrayBuffer from Http2Session/node_http2.cc so i believe the error has to do with it. So i want to make sure that the ArrayBuffer under stream_buf_ab_ is detached right before Reset() is called. I ran all tests and they all passed, did i miss something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so stream_buf_ab_ is the only ArrayBuffer from Http2Session/node_http2.cc so i believe the error has to do with it.

There’s the one that’s being changed here above, too (the one for js_fields_). I think that’s the problematic one, because we allocate the Http2Session ourselves but don’t invalidate/detach the ArrayBuffer when the underlying Http2Session memory is freed.

I think that means that it would need to be stored on the Http2Session in a v8::Global, yes.

I ran all tests and they all passed, did i miss something?

Hm, that’s surprising … maybe our tests just don’t verify this :/ I’ll see if I can come up with something for that…

Copy link
Member

@addaleax addaleax Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran all tests and they all passed, did i miss something?

Hm, that’s surprising … maybe our tests just don’t verify this :/ I’ll see if I can come up with something for that…

I did some digging here and it turns out this was still working because stream_buf_ab_ is never weak, so ClearWeak() always returned nullptr, and the array buffer never actually got detached.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means js_fields_ is our main suspect here? I'm working on a fix for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly – I haven’t verified that it’s the thing that caused the crashes earlier in CI, but it is something that could cause crashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the block of changes here? As mentioned above, it doesn’t do anything right now :)

@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

Do the changes in test/js-native-api mean that there is a beraking change for N-API ?

Hm … I guess this is a breaking change for addons that return an ArrayBuffer for static data, yes.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2019
@Trott
Copy link
Member

Trott commented Dec 6, 2019

@nodejs/n-api

@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 6, 2019
@addaleax addaleax closed this Dec 12, 2019
@BridgeAR
Copy link
Member

The CI currently has lots of crashes related to workers. I guess that it might be related to this PR.

Example failure: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/343/RUN_SUBSET=3,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_worker_error_stack_getter_throws/

@addaleax
Copy link
Member

@BridgeAR @nodejs/build Core dumps might be massively helpful here, once again :)

I’ll take a look and see if I can reproduce something and/or see if there was anything that we missed.

addaleax added a commit to addaleax/node that referenced this pull request Dec 13, 2019
This fixes flaky tests that crashed because the allocations ended
up at positions of previously allocated `ArrayBuffer`s that were
still in the backing store table. In particular, there was a race
condition window between destroying a Worker thread’s `Environment`
and destroying its `Isolate` in which the underlying memory was
already released but the `ArrayBuffer` was still existent, meaning
that new memory could be allocated at the address of the previous
`ArrayBuffer`.

Refs: nodejs#30782
@addaleax
Copy link
Member

@BridgeAR @thangktran #30946 should address those failures, please take a look :)

CHECK_NOT_NULL(deleter_data);

static_cast<v8::ArrayBuffer::Allocator*>(deleter_data)
->Free(data, length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s an issue with this that I didn’t realize during review, sorry … see https://bugs.chromium.org/p/v8/issues/detail?id=9908#c10 for details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax should i force-push here or open a new PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thangktran I think neither. I believe @addaleax already opened a PR to fix: #30946

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott i checked it just now and it's actually still there.

Copy link
Member

@addaleax addaleax Dec 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott This here is a V8 API issue, #30946 fixes a different bug in our code. So there is something we need to do.

I think the 3 options here are:

  • Temporarily reverting the changes that add the v8::ArrayBuffer::Allocator::Free calls and go back to using the deprecated APIs, effectively blocking the upgrade to 8.0 or 8.1 (not sure which)
  • Modifying our embedder API to let embedders and ourselves pass a std::shared_ptr<v8::ArrayBuffer::Allocator> that is also passed to V8 and then use a pointer to that instead of a raw v8::ArrayBuffer::Allocator* (I don’t like this one, it’s quite a bit of work and complexity with little gain)
  • Implementing the V8 API suggestion in the V8 bug linked above and then using that. I think that would be the ideal one, but let’s wait for @ulan’s response in the issue first. @thangktran If this works out and you haven’t contributed to V8 before, I’m happy to help answer any questions – I know their process can be a bit unusual for those who aren’t used to it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax : Thank you for offering me help. I'm looking forward to it :D
What is your opinion about @ulan 's alternatives? :

Before we go ahead with the new constructor, I want to discuss the feasibility of the third option:

  1. Change AllocatedBuffer [1] to allocate and keep std::unique_ptrv8::BackingStore.
  2. Add Reallocate method to ArrayBufferAllocator (the default implementation would simply allocate a new buffer).
  3. Add a static Reallocate method to v8::BackingStore that takes a unique_ptr and returns a unique_ptr. Internally it calls Reallocate of the array buffer allocator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thangktran Thanks for the ping! I’ve commented in the issue. Basically, I’m good with Ulan’s suggestion. 👍

gabrielschulhof pushed a commit that referenced this pull request Dec 14, 2019
This fixes flaky tests that crashed because the allocations ended
up at positions of previously allocated `ArrayBuffer`s that were
still in the backing store table. In particular, there was a race
condition window between destroying a Worker thread’s `Environment`
and destroying its `Isolate` in which the underlying memory was
already released but the `ArrayBuffer` was still existent, meaning
that new memory could be allocated at the address of the previous
`ArrayBuffer`.

Refs: #30782
PR-URL: #30946
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 5, 2020
It looks like it’s virtually impossible at this point to
create “fake” backing stores for objects that don’t fully
own their memory allocations, like the sub-field `js_fields_`
of `Http2Session`. In particular, it turns out that an
`ArrayBuffer` cannot always be easily separated from its
backing store in that situation through by detaching it.

This commit gives the JS-exposed parts of the class its own
memory allocation and its own backing store, simplifying the
code a bit and fixing flakiness coming from it, at the cost
of one additional layer of indirection when accessing the data.

Refs: nodejs#30782
Fixes: nodejs#31107
addaleax added a commit that referenced this pull request Feb 8, 2020
It looks like it’s virtually impossible at this point to
create “fake” backing stores for objects that don’t fully
own their memory allocations, like the sub-field `js_fields_`
of `Http2Session`. In particular, it turns out that an
`ArrayBuffer` cannot always be easily separated from its
backing store in that situation through by detaching it.

This commit gives the JS-exposed parts of the class its own
memory allocation and its own backing store, simplifying the
code a bit and fixing flakiness coming from it, at the cost
of one additional layer of indirection when accessing the data.

Refs: #30782
Fixes: #31107

PR-URL: #31648
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
BethGriggs added a commit that referenced this pull request Apr 20, 2020
- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL
  (James M Snell) [#31166](#31166)
- (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection
  (James M Snell) [#28396](#28396)
- (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL
  (James M Snell) [#31164](#31164)
- (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL
  (James M Snell) [#31167](#31167)
- (SEMVER-MAJOR) os: move tmpDir() to EOL
  (James M Snell)[#31169](#31169)
- (SEMVER-MAJOR) src: remove deprecated wasm type check
  (Clemens Backes) [#32116](#32116)
- (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL
  (James M Snell) [#31165](#31165)
- (SEMVER-MINOR) doc: deprecate process.mainModule
  (Antoine du HAMEL) [#32232](#32232)
- (SEMVER-MINOR) doc: deprecate process.umask() with no arguments
  (Colin Ihrig) [#32499](#32499)

- src: migrate to new V8 ArrayBuffer API (Thang Tran)
  [#30782](#30782)

It is possible that this change will impact some native addons using
`ArrayBuffer`.

- (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x
  (AshCripps)[#32454](#32454)
- (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: remove SmartOS from official binaries
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) win: block running on EOL Windows versions
  (João Reis) [#31954](#31954)

It is expected that there will be an ABI mismatch on ARM between the
Node.js binary and native addons.
- [#30786](#30786)

- (SEMVER-MAJOR) deps: update V8 to 8.1.307.20
  (Matheus Marchini) [#32116](#32116)

- cli, report: move --report-on-fatalerror to stable
  (Colin Ihrig) [#32496](#32496)
- deps: upgrade to libuv 1.37.0
  (Colin Ihrig) [#32866](#32866)
- fs: add fs/promises alias module
  (Gus Caplan) [#31553](#31553)
- module: remove experimental modules warning
  (Guy Bedford) [#31974](#31974)

PR-URL: #32181
BethGriggs added a commit that referenced this pull request Apr 20, 2020
Deprecations:

- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL
  (James M Snell) [#31166](#31166)
- (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection
  (James M Snell) [#28396](#28396)
- (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL
  (James M Snell) [#31164](#31164)
- (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL
  (James M Snell) [#31167](#31167)
- (SEMVER-MAJOR) os: move tmpDir() to EOL
  (James M Snell)[#31169](#31169)
- (SEMVER-MAJOR) src: remove deprecated wasm type check
  (Clemens Backes) [#32116](#32116)
- (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL
  (James M Snell) [#31165](#31165)
- (SEMVER-MINOR) doc: deprecate process.mainModule
  (Antoine du HAMEL) [#32232](#32232)
- (SEMVER-MINOR) doc: deprecate process.umask() with no arguments
  (Colin Ihrig) [#32499](#32499)

ECMAScript Modules - Experimental Warning Removal:

- module: remove experimental modules warning
  (Guy Bedford) [#31974](#31974)

In Node.js 13 we removed the need to include the --experimental-modules
flag, but when running EcmaScript Modules in Node.js, this would still
result in a warning ExperimentalWarning: The ESM module loader is
experimental.

As of Node.js 14 there is no longer this warning when using ESM in
Node.js. However, the ESM implementation in Node.js remains
experimental. As per our stability index: “The feature is not subject
to Semantic Versioning rules. Non-backward compatible changes or
removal may occur in any future release.” Users should be cautious when
using the feature in production environments.

Please keep in mind that the implementation of ESM in Node.js differs
from the developer experience you might be familiar with. Most
transpilation workflows support features such as optional file
extensions or JSON modules that the Node.js ESM implementation does not
support. It is highly likely that modules from transpiled environments
will require a certain degree of refactoring to work in Node.js. It is
worth mentioning that many of our design decisions were made with two
primary goals. Spec compliance and Web Compatibility. It is our belief
that the current implementation offers a future proof model to
authoring ESM modules that paves the path to Universal JavaScript.
Please read more in our documentation.

The ESM implementation in Node.js is still experimental but we do believe
that we are getting very close to being able to call ESM in Node.js
“stable”. Removing the warning is a huge step in that direction.

Migrate to new V8 ArrayBuffer API:

- src: migrate to new V8 ArrayBuffer API (Thang Tran)
  [#30782](#30782)

It is possible that this change will impact some native addons using
`ArrayBuffer`.

Toolchain and Compiler Upgrades:

- (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x
  (AshCripps)[#32454](#32454)
- (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: remove SmartOS from official binaries
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) win: block running on EOL Windows versions
  (João Reis) [#31954](#31954)

It is expected that there will be an ABI mismatch on ARM between the
Node.js binary and native addons. Native addons are only broken if they
interact with `std::shared_ptr`. This is expected to be fixed in a
later version of Node.js 14.
- [#30786](#30786)

Update to V8 8.1:

- (SEMVER-MAJOR) deps: update V8 to 8.1.307.20
  (Matheus Marchini) [#32116](#32116)

Other Notable Changes:

- cli, report: move --report-on-fatalerror to stable
  (Colin Ihrig) [#32496](#32496)
- deps: upgrade to libuv 1.37.0
  (Colin Ihrig) [#32866](#32866)
- fs: add fs/promises alias module
  (Gus Caplan) [#31553](#31553)

PR-URL: #32181
BethGriggs added a commit that referenced this pull request Apr 21, 2020
Deprecations:

- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL
  (James M Snell) [#31166](#31166)
- (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection
  (James M Snell) [#28396](#28396)
- (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL
  (James M Snell) [#31164](#31164)
- (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL
  (James M Snell) [#31167](#31167)
- (SEMVER-MAJOR) os: move tmpDir() to EOL
  (James M Snell)[#31169](#31169)
- (SEMVER-MAJOR) src: remove deprecated wasm type check
  (Clemens Backes) [#32116](#32116)
- (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL
  (James M Snell) [#31165](#31165)
- (SEMVER-MINOR) doc: deprecate process.mainModule
  (Antoine du HAMEL) [#32232](#32232)
- (SEMVER-MINOR) doc: deprecate process.umask() with no arguments
  (Colin Ihrig) [#32499](#32499)

ECMAScript Modules - Experimental Warning Removal:

- module: remove experimental modules warning
  (Guy Bedford) [#31974](#31974)

In Node.js 13 we removed the need to include the --experimental-modules
flag, but when running EcmaScript Modules in Node.js, this would still
result in a warning ExperimentalWarning: The ESM module loader is
experimental.

As of Node.js 14 there is no longer this warning when using ESM in
Node.js. However, the ESM implementation in Node.js remains
experimental. As per our stability index: “The feature is not subject
to Semantic Versioning rules. Non-backward compatible changes or
removal may occur in any future release.” Users should be cautious when
using the feature in production environments.

Please keep in mind that the implementation of ESM in Node.js differs
from the developer experience you might be familiar with. Most
transpilation workflows support features such as optional file
extensions or JSON modules that the Node.js ESM implementation does not
support. It is highly likely that modules from transpiled environments
will require a certain degree of refactoring to work in Node.js. It is
worth mentioning that many of our design decisions were made with two
primary goals. Spec compliance and Web Compatibility. It is our belief
that the current implementation offers a future proof model to
authoring ESM modules that paves the path to Universal JavaScript.
Please read more in our documentation.

The ESM implementation in Node.js is still experimental but we do believe
that we are getting very close to being able to call ESM in Node.js
“stable”. Removing the warning is a huge step in that direction.

Migrate to new V8 ArrayBuffer API:

- src: migrate to new V8 ArrayBuffer API (Thang Tran)
  [#30782](#30782)

It is possible that this change will impact some native addons using
`ArrayBuffer`.

Toolchain and Compiler Upgrades:

- (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x
  (AshCripps)[#32454](#32454)
- (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: remove SmartOS from official binaries
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) win: block running on EOL Windows versions
  (João Reis) [#31954](#31954)

It is expected that there will be an ABI mismatch on ARM between the
Node.js binary and native addons. Native addons are only broken if they
interact with `std::shared_ptr`. This is expected to be fixed in a
later version of Node.js 14.
- [#30786](#30786)

Update to V8 8.1:

- (SEMVER-MAJOR) deps: update V8 to 8.1.307.20
  (Matheus Marchini) [#32116](#32116)

Other Notable Changes:

- cli, report: move --report-on-fatalerror to stable
  (Colin Ihrig) [#32496](#32496)
- deps: upgrade to libuv 1.37.0
  (Colin Ihrig) [#32866](#32866)
- fs: add fs/promises alias module
  (Gus Caplan) [#31553](#31553)

PR-URL: #32181
BethGriggs added a commit that referenced this pull request Apr 21, 2020
Deprecations:

- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL
  (James M Snell) [#31166](#31166)
- (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection
  (James M Snell) [#28396](#28396)
- (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL
  (James M Snell) [#31164](#31164)
- (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL
  (James M Snell) [#31167](#31167)
- (SEMVER-MAJOR) os: move tmpDir() to EOL
  (James M Snell)[#31169](#31169)
- (SEMVER-MAJOR) src: remove deprecated wasm type check
  (Clemens Backes) [#32116](#32116)
- (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL
  (James M Snell) [#31165](#31165)
- (SEMVER-MINOR) doc: deprecate process.mainModule
  (Antoine du HAMEL) [#32232](#32232)
- (SEMVER-MINOR) doc: deprecate process.umask() with no arguments
  (Colin Ihrig) [#32499](#32499)

ECMAScript Modules - Experimental Warning Removal:

- module: remove experimental modules warning
  (Guy Bedford) [#31974](#31974)

In Node.js 13 we removed the need to include the --experimental-modules
flag, but when running EcmaScript Modules in Node.js, this would still
result in a warning ExperimentalWarning: The ESM module loader is
experimental.

As of Node.js 14 there is no longer this warning when using ESM in
Node.js. However, the ESM implementation in Node.js remains
experimental. As per our stability index: “The feature is not subject
to Semantic Versioning rules. Non-backward compatible changes or
removal may occur in any future release.” Users should be cautious when
using the feature in production environments.

Please keep in mind that the implementation of ESM in Node.js differs
from the developer experience you might be familiar with. Most
transpilation workflows support features such as optional file
extensions or JSON modules that the Node.js ESM implementation does not
support. It is highly likely that modules from transpiled environments
will require a certain degree of refactoring to work in Node.js. It is
worth mentioning that many of our design decisions were made with two
primary goals. Spec compliance and Web Compatibility. It is our belief
that the current implementation offers a future proof model to
authoring ESM modules that paves the path to Universal JavaScript.
Please read more in our documentation.

The ESM implementation in Node.js is still experimental but we do believe
that we are getting very close to being able to call ESM in Node.js
“stable”. Removing the warning is a huge step in that direction.

New V8 ArrayBuffer API:

* **src**: migrate to new V8 ArrayBuffer API
  (Thang Tran) [#30782](#30782)

Multiple ArrayBuffers pointing to the same base address are no longer
allowed by V8. This may impact native addons.

Toolchain and Compiler Upgrades:

- (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x
  (AshCripps)[#32454](#32454)
- (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: remove SmartOS from official binaries
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) win: block running on EOL Windows versions
  (João Reis) [#31954](#31954)

It is expected that there will be an ABI mismatch on ARM between the
Node.js binary and native addons. Native addons are only broken if they
interact with `std::shared_ptr`. This is expected to be fixed in a
later version of Node.js 14.
- [#30786](#30786)

Update to V8 8.1:

- (SEMVER-MAJOR) deps: update V8 to 8.1.307.20
  (Matheus Marchini) [#32116](#32116)

Other Notable Changes:

- cli, report: move --report-on-fatalerror to stable
  (Colin Ihrig) [#32496](#32496)
- deps: upgrade to libuv 1.37.0
  (Colin Ihrig) [#32866](#32866)
- fs: add fs/promises alias module
  (Gus Caplan) [#31553](#31553)

PR-URL: #32181
BethGriggs added a commit that referenced this pull request Apr 21, 2020
Deprecations:

- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL
  (James M Snell) [#31166](#31166)
- (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection
  (James M Snell) [#28396](#28396)
- (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL
  (James M Snell) [#31164](#31164)
- (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL
  (James M Snell) [#31167](#31167)
- (SEMVER-MAJOR) os: move tmpDir() to EOL
  (James M Snell)[#31169](#31169)
- (SEMVER-MAJOR) src: remove deprecated wasm type check
  (Clemens Backes) [#32116](#32116)
- (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL
  (James M Snell) [#31165](#31165)
- (SEMVER-MINOR) doc: deprecate process.mainModule
  (Antoine du HAMEL) [#32232](#32232)
- (SEMVER-MINOR) doc: deprecate process.umask() with no arguments
  (Colin Ihrig) [#32499](#32499)

ECMAScript Modules - Experimental Warning Removal:

- module: remove experimental modules warning
  (Guy Bedford) [#31974](#31974)

In Node.js 13 we removed the need to include the --experimental-modules
flag, but when running EcmaScript Modules in Node.js, this would still
result in a warning ExperimentalWarning: The ESM module loader is
experimental.

As of Node.js 14 there is no longer this warning when using ESM in
Node.js. However, the ESM implementation in Node.js remains
experimental. As per our stability index: “The feature is not subject
to Semantic Versioning rules. Non-backward compatible changes or
removal may occur in any future release.” Users should be cautious when
using the feature in production environments.

Please keep in mind that the implementation of ESM in Node.js differs
from the developer experience you might be familiar with. Most
transpilation workflows support features such as optional file
extensions or JSON modules that the Node.js ESM implementation does not
support. It is highly likely that modules from transpiled environments
will require a certain degree of refactoring to work in Node.js. It is
worth mentioning that many of our design decisions were made with two
primary goals. Spec compliance and Web Compatibility. It is our belief
that the current implementation offers a future proof model to
authoring ESM modules that paves the path to Universal JavaScript.
Please read more in our documentation.

The ESM implementation in Node.js is still experimental but we do believe
that we are getting very close to being able to call ESM in Node.js
“stable”. Removing the warning is a huge step in that direction.

New V8 ArrayBuffer API:

* **src**: migrate to new V8 ArrayBuffer API
  (Thang Tran) [#30782](#30782)

Multiple ArrayBuffers pointing to the same base address are no longer
allowed by V8. This may impact native addons.

Toolchain and Compiler Upgrades:

- (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x
  (AshCripps)[#32454](#32454)
- (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) doc: remove SmartOS from official binaries
  (Richard Lau) [#32812](#32812)
- (SEMVER-MAJOR) win: block running on EOL Windows versions
  (João Reis) [#31954](#31954)

It is expected that there will be an ABI mismatch on ARM between the
Node.js binary and native addons. Native addons are only broken if they
interact with `std::shared_ptr`. This is expected to be fixed in a
later version of Node.js 14.
- [#30786](#30786)

Update to V8 8.1:

- (SEMVER-MAJOR) deps: update V8 to 8.1.307.20
  (Matheus Marchini) [#32116](#32116)

Other Notable Changes:

- cli, report: move --report-on-fatalerror to stable
  (Colin Ihrig) [#32496](#32496)
- deps: upgrade to libuv 1.37.0
  (Colin Ihrig) [#32866](#32866)
- fs: add fs/promises alias module
  (Gus Caplan) [#31553](#31553)

PR-URL: #32181
sanosdole added a commit to sanosdole/nodeclrhost that referenced this pull request Mar 30, 2021
Implement support for Electron 12.

Major changes:
- [node #30782](nodejs/node#30782) New ArrayBuffer backing store prevents reusing the same data pointer
- [electron #27949](electron/electron#27949) Context isolation is turned on by default and prevents `coreclr-hosting` from working properly
- [electron #18397](electron/electron#18397) & [electron #454](maximegris/angular-electron#454) lead to `app.allowRendererProcessReuse` being fixed to true on Electron 12. This required making `coreclr-hosting` context aware and implement proper re-use of the hosted CLR.

Known Issues:
- After reloading a renderer app the task scheduler sometimes hangs up, as the the `ThreadSafeFunction` does not get invoked.

Code cleanup and small memory management improvements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants