fix: unexpected null pointer exception due to race condition#217
fix: unexpected null pointer exception due to race condition#217JounQin merged 14 commits intoun-ts:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b701670 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 95.79% 95.82% +0.02%
==========================================
Files 4 4
Lines 333 335 +2
Branches 153 154 +1
==========================================
+ Hits 319 321 +2
+ Misses 14 11 -3
- Partials 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe updates introduce a new reliability test that repeatedly calls a synchronous worker function, add a simple worker identity script, and enhance robustness in message handling within the worker communication logic. Additionally, configuration and documentation files are updated to reflect these changes and address a race condition issue. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as test/reliability.spec.ts
participant MainThread as Main Thread
participant Worker as worker-identity.js
TestRunner->>MainThread: createSyncFn(worker-identity.js)
loop 1,000,000 times
TestRunner->>MainThread: identityFn(i)
MainThread->>Worker: Send message with i
Worker->>MainThread: Return i
MainThread->>TestRunner: Return i
TestRunner->>TestRunner: Assert returned value === i
end
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/helpers.tsOops! Something went wrong! :( ESLint: 9.27.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js test/reliability.spec.tsOops! Something went wrong! :( ESLint: 9.27.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
c3ccc24 to
8f2dae7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR reverts the previous cache initialization change (#156) by eagerly initializing module-level caches and adds a high-iteration reliability test to catch race-condition–related null pointer errors.
- Initialize
syncFnCacheandglobalsCacheas non-undefined Maps to avoid race-condition nulls. - Add a new
worker-identity.jsandreliability.spec.tsto stress-testcreateSyncFnreturns. - Ensure the fix is applied to both mainline and 0.9.x/0.10.x branches.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/worker-identity.js | Introduces a basic worker identity function test |
| test/reliability.spec.ts | Adds a looped reliability test for createSyncFn |
| src/index.ts | Changes syncFnCache from lazy to eager init |
| src/helpers.ts | Changes globalsCache from lazy to eager init |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 4fac75c in 1 minute and 24 seconds. Click for details.
- Reviewed
90lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/sharp-olives-itch.md:1
- Draft comment:
Changelog looks clear and the version bump is appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/helpers.ts:396
- Draft comment:
Pre-initialize globalsCache as a constant to avoid lazy initialization race conditions. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src/index.ts:29
- Draft comment:
Similarly, pre-initializing syncFnCache avoids potential race conditions caused by lazy initialization. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining why pre-initializingsyncFnCacheis beneficial. It doesn't suggest a change or ask for confirmation of intent. According to the rules, purely informative comments should be removed.
4. test/reliability.spec.ts:6
- Draft comment:
The reliability test is a strong addition; however, verify that 1e6 iterations under CI don’t cause performance issues or CI timeouts. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. test/worker-identity.js:1
- Draft comment:
Worker identity file is minimal and correct, echoing the input via runAsWorker. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_daYwOxY3KQw0e3Tb
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/reliability.spec.ts (1)
6-6: Consider adding timeout protection for the reliability test.While the environment-based iteration count addresses previous feedback about performance, the test lacks timeout protection which could cause indefinite hangs if the race condition being tested manifests as a deadlock.
Consider adding a test timeout or making the iteration count more configurable:
// eslint-disable-next-line @typescript-eslint/no-magic-numbers -const times = process.env.CI ? 1e6 : 1e3 +const times = process.env.TEST_ITERATIONS + ? parseInt(process.env.TEST_ITERATIONS, 10) + : process.env.CI ? 1e6 : 1e3
🧹 Nitpick comments (1)
test/reliability.spec.ts (1)
8-20: Enhance the reliability test with better error reporting and timeout protection.The test structure is solid for detecting race conditions, but could benefit from improvements to make debugging easier and prevent indefinite hangs.
Consider these enhancements:
-test(`Reliability (${times} runs)`, async () => { +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 { expect(identity(index)).toBe(index) } catch (error) { - console.log(`Failed on ${index + 1} run.`) + console.error(`Failed on run ${index + 1}/${times}. Input: ${index}, Error:`, error) throw error } } -}) +}, 60000) // 60 second timeout to prevent indefinite hangsThis adds:
- More detailed error logging including the error details
- A reasonable timeout to prevent CI hangs if the race condition causes deadlocks
- Better context in the error message
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/sharp-olives-itch.md(1 hunks)src/helpers.ts(1 hunks)src/index.ts(1 hunks)test/reliability.spec.ts(1 hunks)test/worker-identity.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/worker-identity.js (1)
src/index.ts (1)
runAsWorker(96-149)
test/reliability.spec.ts (2)
src/index.ts (1)
createSyncFn(50-78)test/helpers.ts (1)
_dirname(9-9)
src/index.ts (1)
src/types.ts (1)
AnyFn(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 23 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
src/helpers.ts (1)
399-399: Excellent fix for the race condition!Eagerly initializing
globalsCacheas a constant Map eliminates the race condition that could occur when multiple threads attempted to initialize the cache simultaneously using the previous lazy initialization pattern. This directly addresses the null pointer exception issue described in the PR objectives..changeset/sharp-olives-itch.md (1)
1-6: Well-documented changeset entry.The changeset accurately describes the patch addressing the race condition that caused null pointer exceptions. The format and content are appropriate for a patch-level change.
test/worker-identity.js (1)
1-4: Simple and effective reliability test worker.The identity function implementation is perfect for testing the worker thread mechanism and detecting race conditions without introducing complexity. This complements the reliability tests mentioned in the AI summary.
src/index.ts (1)
32-32: Consistent race condition fix applied.Eagerly initializing
syncFnCacheas a constant Map mirrors the same pattern applied insrc/helpers.tsand effectively prevents the race condition that could occur during lazy initialization. This change directly contributes to resolving the null pointer exceptions mentioned in the PR objectives.test/reliability.spec.ts (1)
1-3: LGTM! Clean and appropriate imports.The imports are well-structured with proper separation between Node.js built-in modules and local helpers.
d7ecceb to
6f45612
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
147-147: Review ignoreFiles pattern: broad directory ignore may hide relevant files
ChangingignoreFilesfrom"**/*.d.ts"to"lib"will exclude the entire compiled output directory—including any new file types you might unintentionally place there—from type coverage. Consider narrowing the pattern to only ignore declaration files, e.g.:"typeCoverage": { "ignoreFiles": [ - "lib" + "lib/**/*.d.ts" ], … }This ensures that only
.d.tsartifacts are skipped, while still auditing any other files underlib.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/shiny-kids-check.md(1 hunks)package.json(1 hunks)src/helpers.ts(2 hunks)test/reliability.spec.ts(1 hunks)test/types-d.cts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .changeset/shiny-kids-check.md
- test/types-d.cts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/helpers.ts
- test/reliability.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18.18 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on ubuntu-latest
This PR fixes #185 which is related to #184 that introduced possible
undefinedmessages fromporttomain, 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
Important
Fixes race condition causing null pointer exception by reverting #156 and adding a reliability test.
globalsCacheandsyncFnCacheto always initialize asMapinhelpers.tsandindex.ts.reliability.spec.tsto run 1,000 or 1,000,000 iterations based on environment.worker-identity.jsfor identity function testing..changeset/sharp-olives-itch.mdto document the fix.This description was created by
for 4fac75c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests
Chores