Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Incorporate worker thread id in temporary filename #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wbinnssmith
Copy link

@wbinnssmith wbinnssmith commented May 18, 2020

What / Why

With the addition of Worker Threads [0] in Node, multiple concurrent threads could attempt to write to the same temporary filename, given they share the same process.pid, and could share the same invocation count as well. This incorporates the thread id, exposed from the worker_threads exports as threadId.

The "main" process does not have a threadId, so -1 is used as its id in that case, as well as in the case the worker_threads module isn't available, such as in older versions of node.

Test Plan

Added a test for normalizing the threadId.

References

[0] https://nodejs.org/api/worker_threads.html

cc @isaacs @aeschright @padmaia @devongovett @mischnic

@@ -0,0 +1,3 @@
'use strict'

require('worker_threads').parentPort.postMessage(require('../thread-id'))
Copy link
Author

Choose a reason for hiding this comment

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

I tried inlining this fixture into the tests using the eval option when constructing the Worker, but this require needs to be resolved from somewhere. Couldn't find a way to set that if it's possible.

@wbinnssmith
Copy link
Author

Looks like worker threads were marked as experimental through Node 11 and that the environments that Travis tests likely don't have them at all, or they're behind an experimental flag.

The environments should probably be updated to include newer versions of Node to exercise this behavior (and maybe drop unmaintained versions <= Node 8)

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

Successfully merging this pull request may close these issues.

1 participant