Skip to content

Commit

Permalink
feat(zero-cache): make forking processes work on windows
Browse files Browse the repository at this point in the history
  • Loading branch information
@tantaman
tantaman committed Dec 24, 2024
1 parent 2f179e0 commit 576f796
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 55 deletions.
2 changes: 1 addition & 1 deletion apps/zbugs/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"@tailwindcss/forms": "^0.5.0",
"@types/js-cookie": "^3.0.6",
"@types/k6": "^0.53.2",
"@types/node": "^18.16.0",
"@types/node": "^20.8.4",
"@types/react": "18.2.13",
"@types/react-dom": "18.2.6",
"@types/react-window": "^1.8.8",
Expand Down
78 changes: 28 additions & 50 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/zero-cache/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"@types/lodash.kebabcase": "^4.1.9",
"@types/lodash.merge": "^4.6.9",
"@types/lodash.snakecase": "^4.1.9",
"@types/node": "^18.16.0",
"@types/node": "^20.8.4",
"@types/pg": "^8.11.2",
"@types/pg-format": "^1.0.5",
"@types/strip-ansi": "^3.0.0",
Expand Down
6 changes: 3 additions & 3 deletions packages/zero-cache/src/types/processes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,13 +159,13 @@ export function childWorker(
const ext = path.extname(import.meta.url);
// modulePath is .ts. If we have been compiled, it should be changed to .js
modulePath = modulePath.replace(/\.ts$/, ext);
const absModulePath = new URL(`../${modulePath}`, import.meta.url).pathname;
const moduleUrl = new URL(`../${modulePath}`, import.meta.url);

args.push(...process.argv.slice(2));

if (singleProcessMode()) {
const [parent, child] = inProcChannel();
import(absModulePath)
import(moduleUrl.href)
.then(({default: runWorker}) =>
runWorker(parent, env ?? process.env, ...args).then(
() => child.emit('close', 0),
Expand All @@ -176,7 +176,7 @@ export function childWorker(
return child;
}
return wrap(
fork(absModulePath, args, {
fork(moduleUrl, args, {

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 25, 2024

Contributor

This breaks typing:

Argument of type `URL` is not assignable to parameter of type `string`

This works:

...
const child = fork(fileURLToPath(moduleUrl.href), args, {
...

Image

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 26, 2024

Contributor

@tantaman

This comment has been minimized.

Sign in to view

Copy link
@tantaman

tantaman Dec 26, 2024

Author Contributor

Npm install and your type errors will go away. We require node 20 and node 20 supports a url here.

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 26, 2024 •
edited
Loading

Contributor

will test again, I'm on node 22

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 26, 2024

Contributor

yup, that worked, Thanks!
detached: true, // do not automatically propagate SIGINT
serialization: 'advanced', // use structured clone for IPC
env,

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 25, 2024 •
edited
Loading

Contributor

The default child process output piping breaks the main process on Windows. Passing silent: true fixes it.

Child process stdout and stderr remain available as node:stream/Readable instances off of the ChildProcess instance returned by fork.

In #3452 I refactored this method to look like this, which allows debugging output from workers:

  const child = fork(fileURLToPath(moduleUrl.href), args, {
    detached: true, // do not automatically propagate SIGINT
    serialization: 'advanced', // use structured clone for IPC
    env,
    // child process stout/stderr piping breaks the main process on Windows
    silent: true,
  });

  // uncomment these lines to debug child process output
  const onWorkerOutput = (data: Buffer) => {
    const msg = data.toString('utf8');
    console.log(msg);
  };
  child.stdout?.on('data', onWorkerOutput);
  child.stderr?.on('data', onWorkerOutput);

  return wrap(child);

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 26, 2024

Contributor

@tantaman

This comment has been minimized.

Sign in to view

Copy link
@tantaman

tantaman Dec 26, 2024

Author Contributor

Yeah I haven’t incorporated all your changes yet. Was still trying to understand why it breaks and how changing it impacts other environments. And been on holiday so a bit slow

This comment has been minimized.

Sign in to view

Copy link
@cbnsndwch

cbnsndwch Dec 26, 2024

Contributor

no worries!
Expand Down

0 comments on commit 576f796

Please sign in to comment.