Fix worker threads crash

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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

[mohd-akram]

Fixes#1365.

[jschlight]

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]

@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]

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]

--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}'

[davedoesdev]

You should check constructor isn't null before dereferencing it.

[mohd-akram]

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

[davedoesdev]

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

[mohd-akram]

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.

[davedoesdev]

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]

Thank you.
5.0.2 👍

[zaquas77]

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]

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