Skip to content

fix: accept process.env and SHARE_ENV in options.env - #138

Open
shaurya703 wants to merge 1 commit into
tinylibs:mainfrom
shaurya703:fix/env-type-share-env
Open

fix: accept process.env and SHARE_ENV in options.env#138
shaurya703 wants to merge 1 commit into
tinylibs:mainfrom
shaurya703:fix/env-type-share-env

Conversation

@shaurya703

Copy link
Copy Markdown

Fixes #136

Not a purposeful restriction as far as I can tell — ThreadWorker already passes options straight to new Worker, so both values worked at runtime and only the type refused them. options.env now mirrors WorkerOptions['env']:

env?: NodeJS.Dict<string> | typeof SHARE_ENV

in Options and in TinypoolWorker['initialize']. process.env is Record<string, string | undefined>, which is why the old Record<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. ProcessWorker forks with:

env: { ...options.env, TINYPOOL_WORKER_ID:  }

and { ...aSymbol } is {}. 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. Widening the type alone would have made that reachable from a type-checking program.

tsc says so the moment the type admits a symbol, which is a nice confirmation rather than something I had to reason about:

src/runtime/process-worker.ts: error TS2698: Spread types may only be created
from object types.

So the combination is refused, not approximated. assertEnvRuntime throws a TypeError naming the fix, and it runs in two places:

  • the constructor, for the obvious case;
  • recycleWorkers, because it is the one path that moves an existing pool onto child_process — a constructor-only guard is bypassed by pool.recycleWorkers({ runtime: 'child_process' }).

ProcessWorker additionally 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. The SHARE_ENV one 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:

change result
constructor guard removed refuses SHARE_ENV with runtime child_processred
recycleWorkers guard removed refuses recycling a SHARE_ENV pool onto child_processred
env reverted to Record<string, string> the issue's own two errors, against this suite: Type 'ProcessEnv' is not assignable… and Type '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_ENV pool within worker_threads still succeeds — so the guard refuses the runtime that cannot share rather than refusing recycling in general.

98 tests pass. npm run build, npm run typecheck and npm run lint (eslint --max-warnings=0) are all clean.

One open question for you

I refused SHARE_ENV + child_process rather than silently falling back to process.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 for child_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.

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.
@shaurya703

Copy link
Copy Markdown
Author

@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 NodeJS.ProcessEnv and deliberately leaves SHARE_ENV out because of the child_process runtime. That reasoning is sound, and importantly it is also safe: ProcessEnv contains no symbol, so it does not reach the spread hazard below at all.

What this PR has that #137 does not, in case any of it is wanted:

  1. SHARE_ENV support, which is the second half of what Tinypool options.env type doesn't accept process.env or SHARE_ENV #136 asks about. ThreadWorker passes options straight to new Worker, so it already works at runtime under worker_threads.

  2. A hazard that only appears if you do accept the symbol. ProcessWorker forks with { ...options.env, TINYPOOL_WORKER_ID }, and { ...aSymbol } is {} — a child_process pool given SHARE_ENV would start children with no environment beyond the worker id, no PATH, no HOME, and no error. This is presumably the concrete form of the concern behind fix: accept process.env in worker options #137's decision to leave it out. tsc catches it the moment the type admits a symbol (TS2698: Spread types may only be created from object types).

  3. A guard rather than a fallback, in the constructor and in recycleWorkers — the latter because pool.recycleWorkers({ runtime: 'child_process' }) can move an existing pool onto the runtime that cannot share, which a constructor-only check misses.

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.

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.

Tinypool options.env type doesn't accept process.env or SHARE_ENV

1 participant