Fix ObjectWrap destructor from env exit under node_g

[davedoesdev]

Only fails under debug buildtype (node_g)

Update: Also fails using release 12.16.3

#722

[mhdawson]

@davedoesdev thanks for submitting a PR for this.

Does the stack trace suggest is occurring from a path triggered by the gc. I've not had time yet but wanted to think through if it was a concern or not if that was occurring (also away for next few days so will probably be mid next week before I find time).

[davedoesdev]

@mhdawson it looks like it's when the environment is closing due to the worker thread exiting.
The handles are being cleaned up at that point.

[davedoesdev]

@mhdawson the test also fails under Node 12.16.3 release build!

[mhdawson]

Spent some time looking at this see nodejs/node#33508

[mhdawson]

@davedoesdev nodejs/node#33508 just landed which should fix the reported problem (it will need to land in 14.x and 12.x as well).

I still think adding the test on the node-addon-api side makes sense in addition to the one in core. Can you remove the non-test change and then we'll get it landed once the core backports are complete.

[davedoesdev]

@mhdawson done

[mhdawson]

@davedoesdev thanks, may be a while before we can land as we have to wait for Node.js backports but thanks for all of your working finding and helping to resolve. Much appreciated.

[davedoesdev]

@mhdawson no worries. Any rough ETA?

[mhdawson]

@davedoesdev sorry for the late update. Looks like its been backported as far back as 12.x. I can't remember if we thought it would apply to 10.x but if you update the PR to just add the test we can run the CI and confirm one way or the other, and if not land it.

[davedoesdev]

@mhdawson PR should already have been updated to contain just the test

[mhdawson]

LGTM

[mhdawson]

CI run on 10.x, https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/2666/

[mhdawson]

@davedoesdev any chance you can rebase seems like it needs one now.

[mhdawson]

Failure on windows for different test so unrelated to this PR. @gabrielschulhof would this be related to any of the recent finalizer/timing changes you had looked at?

assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ {}
- {
-   finalizerCalled: true
- }
    at test (C:\workspace\node-test-node-addon-api-new\nodes\win-vs2019\node-addon-api\test\object\finalizer.js:16:10)
    at Object.<anonymous> (C:\workspace\node-test-node-addon-api-new\nodes\win-vs2019\node-addon-api\test\object\finalizer.js:6:1)
    at Module._compile (internal/modules/cjs/loader.js:1089:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1110:10)
    at Module.load (internal/modules/cjs/loader.js:954:32)
    at Function.Module._load (internal/modules/cjs/loader.js:795:14)
    at Module.require (internal/modules/cjs/loader.js:978:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at C:\workspace\node-test-node-addon-api-new\nodes\win-vs2019\node-addon-api\test\index.js:97:5
    at Array.forEach (<anonymous>) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: {},
  expected: { finalizerCalled: true },
  operator: 'deepStrictEqual'
}

[mhdawson]

That same test seems to have failed on other platforms as well. Might be related to me running this on the PR branch which may be missing some of the latest changes.

I'll try again after a rebase.

[davedoesdev]

@mhdawson rebased

[mhdawson]

Added commit to only run on Node.js 12 and higher as it requires worker threads

[mhdawson]

Landed as: 518cfdc

[mhdawson]

Thanks for all your work on this @davedoesdev

[davedoesdev]

@mhdawson thanks for merging