Validity of file descriptors in the context of the `SyncKit` worker

[mash-graz]

While debugging the jco preview2-shim unit tests under deno I stumbled over another grave compatibility issue. It's more or less hidden when executed on nodejs, but has its roots in an undeniable design flaw of jco:

It breaks the test case "FS read" in a rather drastic way, when used on deno, but isn't easy to grasp on first sight.

It's caused by the fact, that jco handles some filesystem related tasks (opening of files in particular) within the context of the main instance, while most other io-related operations (reading files etc.) are happening within the context of a worker thread used for SyncKits indirection.

This unmediated transfer and use of open file descriptors on both sides works in nodejs and bun at least somehow (well -- even there it's confusing the garbage collector and causing subtle troubles), but not on deno and it's much more secure/restrictive/efficient multi threading implementation.

The problem isn't caused by the fact, that files descriptors are used for the communication between both realms -- no, they are just simple numbers --, but the file related operations, which they belong to, have to happen always in the same context, otherwise they become silently invalid!

You shouldn't open the files on one side and expect, that their file descriptor values are valid in other contexts as well. You simply can read from such a received file descriptor number behind the wall, which divides both threads/subprocesses/etc.

Here is a very simple script to test and demonstrate this issue:

/// cross_realm_fd_abuse.mjs

import { isMainThread, parentPort, Worker } from "node:worker_threads";
import fs from "node:fs";

if (isMainThread) { // --- code running on the main instance ---
 
  // open a File in the contex if the Main Thread
  const fd = fs.openSync(import.meta.filename);

  // run this script also as 'node:worker_thread'
  const w = new Worker(import.meta.filename);

  // send file descriptor to the worker side
  console.log(`Send open file descriptor: ${fd}`);
  w.postMessage(fd)

} else { // --- code running on the worker side ---

  // receive file descriptor and try to read file content
  parentPort.on("message", (msg) => {
    const received_fd = msg;
    console.log(`Received file descriptor:  ${received_fd}`);

    fs.read(received_fd, (err, br, buf) => {
      if (err) {
        console.log(`ERROR: ${err}`)
      } else {
        console.log(`READ: ${br} bytes`)
      }

      // cleanup and exit worker
      globalThis.Deno ? close() : parentPort.unref();
    });
  });
}

While it will somehow work on node and bun it will immediately report ERROR: BadResource: Bad resource ID if you try to execute it via deno run -A ./cross_realm_fd_reuse.mjs.

IMHO it's a really significant defect on jcos side, which should be carefully fixed to archive a more reliable and compatible code base. You just have to handle all file related operations in the same execution context resp. in the same manner.

[guybedford]

Thanks for tracking this down, we can certainly move all the FS descriptor operations into the worker thread for Deno support.

I've created a new deno support label and added that here.

[mash-graz]

we can certainly move all the FS descriptor operations into the worker thread for Deno support.

This would really make a lot of sense, because it definitely would help to avoid some subtle troubles even when used in node.

I'm still working on this deno adaptation. Most of the preview2-shim unit-tests already work fine in my patched setup. But I also had to modify deno to archive this results (for example:https://github.com/denoland/deno/pull/22766).

I'll try to send you another bunch of compatibility patches soon. But it'll take a while until everything works...

[guybedford]

Sounds great. It's been fantastic to follow your progress on this. Please just let me know if I can help or review anytime.