APIs removed in V8 7.0 and native addons #23122

addaleax · 2018-09-27T13:10:21Z

Currently, CITGM failures are dominated by addons failing because of APIs that were removed in V8 7.0 (Example run).

Most of the removed functions are simple wrappers that take now-required arguments from other sources, for example, the now-removed value->ToString() was essentially just a shorthand for value->ToString(Isolate::GetCurrent()->GetCurrentContext()).FromMaybe(Local<String>()).

Most of these APIs (all widely used ones, at least) do currently not print deprecation warnings when building with Node 10.

So, the question is: What, if anything, do we do about this?

We have the option of maintaining the deprecated APIs ourselves for a while, but I’m not sure how people feel about that.

We can also do ecosystem outreach on our own, but I highly doubt that that is going to restore CITGM results in time for Node 11, and given the large number of addons that are affected by this, we definitely can’t address all cases where this is an issue by ourselves.

The text was updated successfully, but these errors were encountered:

devsnek · 2018-09-27T13:14:10Z

wasn't our big plan always to shim 68 and 69 for 10.x and them push 70 to 11.x?

targos · 2018-09-27T13:18:09Z

@devsnek this issue is about 11.x

targos · 2018-09-27T13:37:20Z

/cc @nodejs/v8-update

bnoordhuis · 2018-09-27T20:03:43Z

Most of these APIs (all widely used ones, at least) do currently not print deprecation warnings when building with Node 10.

What if we float a patch that changes that? Doesn't break ABI but should result in a flurry of bug reports against add-ons that need upgrading.

I highly doubt that that is going to restore CITGM results in time for Node 11

If 2018-10-23 is still the target date then I agree. Floating shims for a release cycle doesn't trouble me.

addaleax · 2018-09-28T00:35:13Z

The V8 commit in question is v8/v8@5acf205, btw. I don’t know if there are others that are relevant.

Most of these APIs (all widely used ones, at least) do currently not print deprecation warnings when building with Node 10.

What if we float a patch that changes that? Doesn't break ABI but should result in a flurry of bug reports against add-ons that need upgrading.

That seems like a good idea. Looking into it, it’s doable but it’s not trivial because some of the replacement APIs don’t even exist in Node 10.

refack · 2018-09-28T02:00:06Z

BTW: 7.0 is not stable yet, we could petition for reverting of v8/v8@5acf205.

addaleax · 2018-09-28T02:02:46Z

@nodejs/v8 ^^^

hashseed · 2018-09-28T11:57:34Z

I just took a look. It seems the change in question is a mix of removing APIs that are marked V8_DEPRECATED and V8_DEPRECATE_SOON. We are conforming to the policy of marking a deprecated API for at least one version before removing. For the latter, we unfortunately are not.

@danelphick has offered to partially revert this change to address the latter. I.e. instead of removing, we would move them to V8_DEPRECATED. We will still remove APIs that were already marked V8_DEPRECATED though.

However, the underlying issue here is that V8 version bumps happen a lot more often than Node.js. If V8 was to accommodate to Node.js' deprecation policy, we would need a whole year to remove an API. We did that previously with the legacy debugging protocol, but I don't think this is something we can do in general.

I think we need an actual discussion on this underlying issue, going forward.

The motivation behind this particular change is to share objects between isolates in order to save memory. That means that where we derived the isolate from the object via GetIsolate(), we no longer can do that and require the embedder to pass the isolate explicitly.

addaleax · 2018-09-28T12:09:09Z

@hashseed Some of the APIs just used Isolate::GetCurrent() under the hood … is that a valid option?

The motivation behind this particular change is to share objects between isolates in order to save memory.

That sounds exciting :)

hashseed · 2018-09-28T12:14:30Z

I think in this particular change it's possible to shim the removed API with a floating patch.

However, this may not solve similar issues in the future.

hashseed · 2018-09-28T12:19:36Z

Would it be possible to restate Node.js' deprecation policy towards native modules to exclude V8's API? Rationale:

It's easier for us :)
Node.js doesn't own V8's API.
NAPI is a viable alternative.
As module author, you could track node's master branch to detect API issues early.

targos · 2018-09-28T12:29:14Z

We have some text in the collaborator guide: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#breaking-changes-and-deprecations

Note that errors thrown, along with behaviors and APIs implemented by dependencies of Node.js (e.g. those originating from V8) are generally not under the control of Node.js and therefore are not directly subject to this policy. However, care should still be taken when landing updates to dependencies when it is known or expected that breaking changes to error handling may have been made. Additional CI testing may be required.

addaleax · 2018-09-28T12:30:39Z

Would it be possible to restate Node.js' deprecation policy towards native modules to exclude V8's API?

I don’t think that’s realistic for frequently-used APIs like some of the ones being removed here… a lot of legacy native addons do not receive much maintenance, and so having a deprecation warning for at least a full Node.js cycle would be ideal…

devsnek · 2018-09-28T12:43:29Z

what if we just float a patch for keeping 11.x unbroken and change our policy and break in 12.x

targos · 2018-09-28T13:57:19Z

This is the diff of deps/v8/include between v10.x-staging and master:

https://gist.github.com/targos/efd41838f402e78c4caa96d4d18168de

danelphick · 2018-09-28T14:41:15Z

As Yang stated above, we're going to put back all the V8_DEPRECATE_SOON methods that were deleted in that change (but instead mark them V8_DEPRECATED). As such I've uploaded https://chromium-review.googlesource.com/c/v8/v8/+/1251422.

refack · 2018-09-28T18:11:46Z

However, the underlying issue here is that V8 version bumps happen a lot more often than Node.js. If V8 was to accommodate to Node.js' deprecation policy, we would need a whole year to remove an API. We did that previously with the legacy debugging protocol, but I don't think this is something we can do in general.

I brought up the request to revert, just as a way to buy a little time (that will allow for a smoother release of node11 with V8 7.0).
IMHO all the points that were raised WRT to V8's deprecation policy are understood, and should be taken into consideration by Node.js.

I'd would like to compliment @targos for the initiative to land 7.0 in master sooner rather than when it went stable (and the TSC for approving).
I believe that should be the policy going forward, so we can face such issue sooner.

joaocgreis · 2018-09-28T18:45:19Z

Most (if not all) of the failures I see in CitGM include NAN. Shouldn't https://github.com/nodejs/nan/ be updated instead of changing anything here?

refack · 2018-09-28T19:09:06Z

digitalinfinity · 2018-09-28T21:32:40Z

With respect to N-API being an alternative for the modules, we're tracking evaluating the impacted modules in nodejs/abi-stable-node#346 - the intention is to evaluate if these are good candidates to move to N-API, and if so, engage with module authors on starting a port. However, N-API adoption is slow so we'll probably have to deal with issues like this for the near term.

refack · 2018-09-28T23:46:39Z

First step nodejs/nan#808
Now we need review, land, publish, then push the above list to update and publish.

addaleax · 2018-10-07T23:22:38Z

Should we consider this resolved, or do people feel that we need to have a larger discussion around our management of V8 API deprecations/changes somewhere?

refack · 2018-10-11T17:21:35Z

IHMO if we turn on V8_IMMINENT_DEPRECATION_WARNINGS for addons (#23414 (comment), #23167), the bigger issue will will be mitigated.

addaleax · 2018-10-11T17:29:10Z

@refack I guess you can feel free to open a PR with that?

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <delphick@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Michael Hablich <hablich@chromium.org> Cr-Commit-Position: refs/branch-heads/7.0@{#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424} Refs: v8/v8@9136dd8 Refs: #23122 PR-URL: #23158 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: #23122 PR-URL: #23158 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell · 2018-10-17T20:09:43Z

@addaleax ... what is the status on this?

addaleax · 2018-10-17T20:13:47Z

@jasnell I think we’re good as far as the situation around v10.x/v11.x is concerned… the rest here is more about future-proofing

jasnell · 2018-10-17T20:15:38Z

Ok, taking it off the 11.0.0 milestone then

* `V8_DEPRECATION_WARNINGS` is here for explicity. It is already defined in `node.gypi`, and `addon.gypi`. * `V8_IMMINENT_DEPRECATION_WARNINGS` added to warn addons authors. * `OPENSSL_THREADS` apears in the openSSL `.h` files, and was only defined via GYP for the node build. PR-URL: nodejs#23426 Fixes: nodejs#23167 Refs: nodejs#23122 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Trott · 2018-11-21T00:41:34Z

Should this remain open? If so, is there anything specific that is actionable at this time?

refack · 2018-11-21T00:49:01Z

We added #23426 which adds warnings for APIs that are V8_IMMINENT_DEPRECATION. This should give addon writers earlier notice about depredations.
I think that was the low hanging fruit.

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: nodejs/node#23122 PR-URL: nodejs/node#23158 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michaël Zasso <targos@protonmail.com> Patch-Filename: deps_provide_more_v8_backwards_compatibility.patch

BridgeAR · 2020-01-04T02:51:51Z

There was no activity for over a year and we are now at V8 8.0. I guess this is resolved and I am closing this. Please reopen in case there is more left to do.

Several API calls from V8 were deprecated in version 7.0 (that ships w/ node 12), this commit replaces then Refs: nodejs/node#23122 nodejs/node#23159 bcoin-org/bcrypto#7

addaleax added v8 engine Issues and PRs related to the V8 dependency. addons Issues and PRs related to native addons. labels Sep 27, 2018

addaleax added this to the 11.0.0 milestone Sep 27, 2018

refack mentioned this issue Sep 28, 2018

deps: adapt to V8 7.0 nodejs/nan#808

Merged

addaleax mentioned this issue Sep 29, 2018

src: use JS inheritance for AsyncWrap #23094

Closed

3 tasks

refack mentioned this issue Oct 11, 2018

deps: update V8 to 7.1 #23423

Closed

refack mentioned this issue Oct 11, 2018

build: add common defines #23426

Merged

3 tasks

jasnell pushed a commit that referenced this issue Oct 17, 2018

deps: patch V8 to 7.0.276.24

d25646b

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell removed this from the 11.0.0 milestone Oct 17, 2018

chjj mentioned this issue Nov 3, 2018

v8/node deprecated APIs and how to handle them bcoin-org/bcrypto#7

Closed

rogerwang mentioned this issue Nov 15, 2018

nw-gyp rebuild nodegit error targeting 0.34.x nwjs/nw.js#6869

Closed

ZJONSSON mentioned this issue Apr 30, 2019

Node 12 compilation problems xmppo/node-expat#195

Closed

kryops mentioned this issue Apr 30, 2019

Node 12 support MadLittleMods/node-usb-detection#80

Closed

BridgeAR closed this as completed Jan 4, 2020

FelipeGdM mentioned this issue Jan 22, 2021

error: ‘v8::Handle’ has not been declared osrf/gzweb#203

Closed

FelipeGdM mentioned this issue Jan 22, 2021

Add support to node 12.x and Gazebo 11 osrf/gzweb#205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIs removed in V8 7.0 and native addons #23122

APIs removed in V8 7.0 and native addons #23122

addaleax commented Sep 27, 2018

devsnek commented Sep 27, 2018

targos commented Sep 27, 2018

targos commented Sep 27, 2018

bnoordhuis commented Sep 27, 2018

addaleax commented Sep 28, 2018

refack commented Sep 28, 2018

addaleax commented Sep 28, 2018

hashseed commented Sep 28, 2018

addaleax commented Sep 28, 2018

hashseed commented Sep 28, 2018

hashseed commented Sep 28, 2018

targos commented Sep 28, 2018

addaleax commented Sep 28, 2018

devsnek commented Sep 28, 2018

targos commented Sep 28, 2018

danelphick commented Sep 28, 2018

refack commented Sep 28, 2018

joaocgreis commented Sep 28, 2018

refack commented Sep 28, 2018

digitalinfinity commented Sep 28, 2018

refack commented Sep 28, 2018

addaleax commented Oct 7, 2018

refack commented Oct 11, 2018

addaleax commented Oct 11, 2018

jasnell commented Oct 17, 2018

addaleax commented Oct 17, 2018

jasnell commented Oct 17, 2018

Trott commented Nov 21, 2018

refack commented Nov 21, 2018

BridgeAR commented Jan 4, 2020

APIs removed in V8 7.0 and native addons #23122

APIs removed in V8 7.0 and native addons #23122

Comments

addaleax commented Sep 27, 2018

devsnek commented Sep 27, 2018

targos commented Sep 27, 2018

targos commented Sep 27, 2018

bnoordhuis commented Sep 27, 2018

addaleax commented Sep 28, 2018

refack commented Sep 28, 2018

addaleax commented Sep 28, 2018

hashseed commented Sep 28, 2018

addaleax commented Sep 28, 2018

hashseed commented Sep 28, 2018

hashseed commented Sep 28, 2018

targos commented Sep 28, 2018

addaleax commented Sep 28, 2018

devsnek commented Sep 28, 2018

targos commented Sep 28, 2018

danelphick commented Sep 28, 2018

refack commented Sep 28, 2018

joaocgreis commented Sep 28, 2018

refack commented Sep 28, 2018

digitalinfinity commented Sep 28, 2018

refack commented Sep 28, 2018

addaleax commented Oct 7, 2018

refack commented Oct 11, 2018

addaleax commented Oct 11, 2018

jasnell commented Oct 17, 2018

addaleax commented Oct 17, 2018

jasnell commented Oct 17, 2018

Trott commented Nov 21, 2018

refack commented Nov 21, 2018

BridgeAR commented Jan 4, 2020