fix: accept process.env and SHARE_ENV in options.env - #138
Conversation
Fixes tinylibs#136 `options.env` was typed `Record<string, string>`, so neither of the values `new Worker(_, { env })` documents could be written down: Type 'ProcessEnv' is not assignable to type 'Record<string, string>' Type 'symbol' is not assignable to type 'Record<string, string>' `process.env` is `Record<string, string | undefined>`, and `SHARE_ENV` is a symbol. `ThreadWorker` already hands `options` straight to `new Worker`, so both worked at runtime and only the type refused them. Now mirrored on `WorkerOptions['env']`: `NodeJS.Dict<string> | typeof SHARE_ENV`, in `Options` and in `TinypoolWorker['initialize']`. Widening the type on its own would ship a silent bug, which is the part the issue anticipates but does not pin down. `ProcessWorker` forks with `{ ...options.env, TINYPOOL_WORKER_ID }`, and spreading a symbol gives `{}` — so a `child_process` pool given `SHARE_ENV` would have started every child with no environment beyond the worker id: no PATH, no HOME, and no error. `tsc` catches it the moment the type admits a symbol: src/runtime/process-worker.ts: error TS2698: Spread types may only be created from object types. So the combination is refused rather than approximated. `assertEnvRuntime` throws a TypeError naming the fix, from the constructor and from `recycleWorkers` — the latter because it is the one path that moves an existing pool onto `child_process`, which a constructor-only guard would not see. `ProcessWorker` also throws if a symbol ever reaches it, so the invariant cannot decay back into an empty environment. Seven tests. `SHARE_ENV` is asserted to actually share — a variable set in the parent *after* the worker starts is read back through the pool, which a copied environment could not do. The guards were checked by removing them: constructor guard removed -> refuses SHARE_ENV with child_process, red recycleWorkers guard removed -> refuses recycling onto child_process, red env type reverted -> the issue's own two errors, on this suite There is also a control that recycling a SHARE_ENV pool within worker_threads still succeeds, so the guard refuses the runtime rather than recycling in general. 98 tests pass; build, `tsc --noEmit` and `eslint --max-warnings=0` clean.
|
@cqxswbc2 got here first with #137 — theirs is timestamped 09:06:56Z, mine 09:12:12Z. I opened this without checking for an existing PR on the issue, which is my mistake; flagging it rather than leaving two PRs sitting on one issue. #137 is correct and is the smaller change. It widens to What this PR has that #137 does not, in case any of it is wanted:
Entirely the maintainers' call, and I have no stake in which lands:
@cqxswbc2 — sorry for the duplicate work, that one is on me for not looking before starting. |
Fixes #136
Not a purposeful restriction as far as I can tell —
ThreadWorkeralready passesoptionsstraight tonew Worker, so both values worked at runtime and only the type refused them.options.envnow mirrorsWorkerOptions['env']:in
Optionsand inTinypoolWorker['initialize'].process.envisRecord<string, string | undefined>, which is why the oldRecord<string, string>rejected it.The part that made this more than a type change
You wrote "supporting SHARE_ENV might not always be possible" — that's right, and it is worse than not possible.
ProcessWorkerforks with:and
{ ...aSymbol }is{}. So achild_processpool givenSHARE_ENVwould have started every child with no environment beyond the worker id — noPATH, noHOME— and no error. Widening the type alone would have made that reachable from a type-checking program.tscsays so the moment the type admits a symbol, which is a nice confirmation rather than something I had to reason about:So the combination is refused, not approximated.
assertEnvRuntimethrows aTypeErrornaming the fix, and it runs in two places:recycleWorkers, because it is the one path that moves an existing pool ontochild_process— a constructor-only guard is bypassed bypool.recycleWorkers({ runtime: 'child_process' }).ProcessWorkeradditionally throws if a symbol ever reaches it, so the invariant can't quietly decay back into an empty environment if a third call path appears.Verification
Seven tests in
test/env.test.ts. TheSHARE_ENVone asserts it actually shares, rather than merely that it was accepted: a variable set in the parent after the worker has started is read back through the pool, which a copied environment could not do.Each guard was checked by removing it, and the type by reverting it:
refuses SHARE_ENV with runtime child_process→ redrecycleWorkersguard removedrefuses recycling a SHARE_ENV pool onto child_process→ redenvreverted toRecord<string, string>Type 'ProcessEnv' is not assignable…andType 'symbol' is not assignable…That last one is the reproduction: the new tests are exactly the code from the issue, so they stop compiling if the type regresses.
There's also a control — recycling a
SHARE_ENVpool withinworker_threadsstill succeeds — so the guard refuses the runtime that cannot share rather than refusing recycling in general.98 tests pass.
npm run build,npm run typecheckandnpm run lint(eslint --max-warnings=0) are all clean.One open question for you
I refused
SHARE_ENV+child_processrather than silently falling back toprocess.env, on the grounds that a pool asking to share an environment and getting a copy is a difference the caller would want to know about. If you'd rather it degrade quietly forchild_process— or accept it and document the copy — say so and I'll change it; it's a one-line switch either way and it's your API.