Repository: un-ts/synckit (ts)

Issue:
random undefined message causes synckit to throw an error
It looks like we are assuming all recieved messages are objects with a `message` property: https://github.com/un-ts/synckit/blob/main/src/index.ts#L548

However, sometimes, randomly, only in CI, it seems to receive `undefined`, which synckit doesn't handle, so it throws an error.

Related: https://github.com/prettier/eslint-plugin-prettier/issues/671


Pull request description:
fix: unexpected null pointer exception due to race condition
This PR fixes #185 which is related to #184 that introduced possible `undefined` messages from `port` to `main`, we handle it gracefully now, and it also adds a new reliability test case to ensure no such regression anymore, this fix will be cherry picked into 0.9.x/0.10.x also.

cc @jedlikowski

<!-- ELLIPSIS_HIDDEN -->

----

> [!IMPORTANT]
> Fixes race condition causing null pointer exception by reverting #156 and adding a reliability test.
> 
>   - **Fix**:
>     - Reverts #156 to address race condition causing null pointer exception.
>     - Changes `globalsCache` and `syncFnCache` to always initialize as `Map` in `helpers.ts` and `index.ts`.
>   - **Testing**:
>     - Adds `reliability.spec.ts` to run 1,000 or 1,000,000 iterations based on environment.
>     - Introduces `worker-identity.js` for identity function testing.
>   - **Misc**:
>     - Updates changeset in `.changeset/sharp-olives-itch.md` to document the fix.
> 
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=un-ts%2Fsynckit&utm_source=github&utm_medium=referral)<sup> for 4fac75cbba7279e6c157e50e44a8fad6736d6a0d. You can [customize](https://app.ellipsis.dev/un-ts/settings/summaries) this summary. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

- **Bug Fixes**
  - Resolved a rare issue that could cause a null pointer exception due to a race condition, improving reliability.

- **Tests**
  - Added a comprehensive reliability test to ensure consistent synchronous function behavior.
  - Updated test imports for improved clarity and maintainability.

- **Chores**
  - Adjusted type coverage settings to ignore the "lib" directory instead of all type definition files.
  - Added documentation for the patch update.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Patch:
diff --git a/.changeset/shiny-kids-check.md b/.changeset/shiny-kids-check.md
new file mode 100644
index 0000000..451ef23
--- /dev/null
+++ b/.changeset/shiny-kids-check.md
@@ -0,0 +1,5 @@
+---
+"synckit": patch
+---
+
+fix: unexpected null pointer exception due to race condition
diff --git a/package.json b/package.json
index 1448da1..b872219 100644
--- a/package.json
+++ b/package.json
@@ -144,7 +144,7 @@
     "detail": true,
     "ignoreAsAssertion": true,
     "ignoreFiles": [
-      "**/*.d.ts"
+      "lib"
     ],
     "ignoreNonNullAssertion": true,
     "showRelativePath": true,
diff --git a/src/helpers.ts b/src/helpers.ts
index 8ffb6f4..e26ad7a 100644
--- a/src/helpers.ts
+++ b/src/helpers.ts
@@ -627,11 +627,13 @@ export function startWorkerThread<T extends AnyFn, R = Awaited<ReturnType<T>>>(
       throw new Error('Internal error: Atomics.wait() failed: ' + status)
     }
 
-    const { id, ...message } = (
-      receiveMessageOnPort(mainPort) as { message: WorkerToMainMessage<R> }
-    ).message
+    const result = receiveMessageOnPort(mainPort) as
+      | { message: WorkerToMainMessage<R> }
+      | undefined
 
-    if (id < expectedId) {
+    const msg = result?.message
+
+    if (msg?.id == null || msg.id < expectedId) {
       const waitingTime = Date.now() - start
       return receiveMessageWithId(
         port,
@@ -640,6 +642,8 @@ export function startWorkerThread<T extends AnyFn, R = Awaited<ReturnType<T>>>(
       )
     }
 
+    const { id, ...message } = msg
+
     if (expectedId !== id) {
       throw new Error(
         `Internal error: Expected id ${expectedId} but got id ${id}`,


Test patch:
diff --git a/test/reliability.spec.ts b/test/reliability.spec.ts
new file mode 100644
index 0000000..74cb12e
--- /dev/null
+++ b/test/reliability.spec.ts
@@ -0,0 +1,22 @@
+import path from 'node:path'
+
+import { _dirname, testIf } from './helpers.js'
+
+const times = 1e6
+
+const test = testIf(!!process.env.CI)
+
+test(`Reliability (${times} runs)`, async () => {
+  const { createSyncFn } = await import('synckit')
+  const identity = createSyncFn(path.resolve(_dirname, `./worker-identity.js`))
+
+  for (let index = 0; index < times; index++) {
+    try {
+      // eslint-disable-next-line jest/no-standalone-expect
+      expect(identity(index)).toBe(index)
+    } catch (error) {
+      console.error(`Failed on ${index + 1}/${times} run.`)
+      throw error
+    }
+  }
+})
diff --git a/test/types-d.cts b/test/types-d.cts
index a6da0e5..2fc9524 100644
--- a/test/types-d.cts
+++ b/test/types-d.cts
@@ -1,6 +1,6 @@
 import expect = require('ts-expect')
 
-import synckit = require('..')
+import synckit = require('synckit')
 
 expect.expectType<expect.TypeEqual<synckit.Syncify<() => true>, () => true>>(
   true,
diff --git a/test/worker-identity.js b/test/worker-identity.js
new file mode 100644
index 0000000..4aa88ba
--- /dev/null
+++ b/test/worker-identity.js
@@ -0,0 +1,3 @@
+import { runAsWorker } from 'synckit'
+
+runAsWorker(value => value)