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

Fix worker threads crash #1367

Merged
merged 1 commit into from Sep 6, 2020
Merged

Fix worker threads crash #1367

merged 1 commit into from Sep 6, 2020

Conversation

mohd-akram
Copy link
Contributor

Fixes #1365.

@jschlight
Copy link
Collaborator

This property of binary in the package.json file creates a native addon that can be used only in those versions of Node that support N-API v6 or later:

"napi_versions": [
  6
]

I suggest changing this value to:

"napi_versions": [
  3,
  6
]

This will cause node-pre-gyp to fire off two builds: One for N-API v3 and another for N-API v6. For each build, the C preprocessor symbol NAPI_VERSION will be set to 3 and 6 respectivly. The NAPI_VERSION symbol can then be used for conditional compilation. The original code can then be compiled when NAPI_VERSION is 3, and the new code compiled when NAPI_VERSION is 6.

This will permit older versions of Node to still use the existing functinality without the context-awareness.

There are more details in the node-pre-gyp documentation.

@mohd-akram
Copy link
Contributor Author

@jschlight Thanks, updated. Just a guess, but It seems that the get_napi_version check here yields an incorrect value when using Electron headers, hence the failures.

@jschlight
Copy link
Collaborator

Yes. node-pre-gyp relies on the version number of the Node process running the build to determine the feasible N-API versions to build. The Electron builds are being performed using Node.js v12.18.3 which supports N-API v6. Therefore, node-pre-gyp is requesting a build for N-API v6 even though the build is using Node.js headers for previous Node.js versions that do not support N-API v6.

node-pre-gyp supports a --target option that lets you specify a Node.js version. The documentation is here. I'm not sure if the N-API build code in node-pre-gyp supports this option. But looking at the comments in the code for get_napi_version does not make me optimistic.

As a first step, you might consider adding a --target option to the Electron builds that specifies the Node.js version of the headers the binary is being built with.

If the builds still fail, I can look into this deeper when I return to the office the week of August 17. It may require an update to node-pre-gyp which I'm happy to pursue.

@mohd-akram
Copy link
Contributor Author

mohd-akram commented Aug 6, 2020

--target was already being passed with the Electron version. I changed the Node.js versions in the CI to match Electron's Node.js versions.

Helper:

curl -Ls https://electronjs.org/headers/index.json |
jq 'sort_by(.version) | .[] | select(.version | inside("8.2.0 8.1.0 8.0.0 7.2.0 7.1.0 7.0.0 6.1.0 6.0.0")) | {version,node}'

@mohd-akram mohd-akram mentioned this pull request Aug 6, 2020
#else
Napi::FunctionReference* constructor =
env.GetInstanceData<Napi::FunctionReference>();
return obj.InstanceOf(constructor->Value());
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check constructor isn't null before dereferencing 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.

It should never be null though. It's set in the Init.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if HasInstance is called for a val which doesn't have instance data in its env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the val's env would be the same env that we add the instance data to. The environment is shared by all the objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, there's no way of passing an object with a different env - that would have to be from a different worker thread which would require serialization.

@kewde kewde merged commit c9caae4 into TryGhost:master Sep 6, 2020
@kewde
Copy link
Collaborator

kewde commented Sep 6, 2020

Thank you.
5.0.2 👍

@zaquas77
Copy link

I'm using threads.js with sqlite3 (through express) inside a worker thread and it's working fine for me so far. But if I call, in the same express project, through a simple route, express crash.

@cirosantilli
Copy link

I and others have observed related crashes at: #1381 BTW unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when used with worker threads
6 participants