Fix double callback invoke on unhandled exception

[adamjmcgrath]

If callback is passed, make the callback execution async so unhandled exceptions don't end up in the catch bock and call the callback a second time.

fixes #527

Summary by CodeRabbit

• Bug Fixes

  • Fixed callback execution to defer to the next event loop cycle for successful operations, ensuring consistent asynchronous behavior and preventing timing-related issues.

• Tests

  • Added test suite covering asynchronous callback execution patterns and error handling scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This change modifies the createOptionalCallbackFunction utility to defer callback invocation to the next event loop tick when a synchronous result is produced, rather than invoking it immediately. New tests verify that callbacks are invoked exactly once and asynchronously.

Changes

Cohort / File(s)	Summary
Callback Execution Timing src/types.ts	Modified synchronous callback invocation to use process.nextTick() for success path, preventing double invocation when callbacks throw unhandled exceptions
Test Coverage test/types-tests.spec.ts	Added new test suite verifying callback invoked exactly once despite unhandled errors and confirming asynchronous execution in success path

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A tick, a tock, a deferral sweet,
No double calls, the flow's complete!
Async await, the callback's way,
Once invoked, no more to pay. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix double callback invoke on unhandled exception' directly describes the main change and matches the primary objective of preventing callback double invocation.
Linked Issues check	✅ Passed	The PR addresses issue #527 by deferring callback execution to the next tick, preventing unhandled exceptions in callbacks from triggering double invocation.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the double callback invocation issue: modifying the callback execution in types.ts and adding comprehensive tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

src/types.ts (1)

257-262: Consider making error path async for consistent timing behavior.

The success path now invokes the callback asynchronously via process.nextTick, but the error path (line 261) still invokes the callback synchronously. This creates inconsistent timing:

fn(args, (err, result) => { /* ... */ });
nextStatement(); // Called BEFORE callback on success, AFTER callback on error

While the error path doesn't have the double-callback problem (exceptions from within a catch block propagate up rather than re-entering it), the timing inconsistency could surprise consumers.

♻️ Suggested fix for consistent async behavior

       try {
         const result = syncVersion(...(args.slice(0, -1) as A));
         process.nextTick(() => possibleCallback(null, result));
       } catch (err) {
-        possibleCallback(err instanceof Error ? err : new Error("Unknown error"));
+        process.nextTick(() =>
+          possibleCallback(err instanceof Error ? err : new Error("Unknown error")),
+        );
       }

test/types-tests.spec.ts (1)

5-51: Tests look good; consider adding error path coverage.

The tests effectively verify the fix for double-callback invocation and the async deferral behavior. The uncaughtException listener management is handled correctly with proper restoration.

Consider adding a test for the error path to ensure consistent behavior when syncFn throws:

📝 Suggested test for error path

it("should invoke callback with error when sync function throws", function (done) {
  const syncFn = () => {
    throw new Error("sync function error");
  };
  const flexibleFn = types.createOptionalCallbackFunction(syncFn);

  let callbackExecuted = false;

  flexibleFn((err, result) => {
    callbackExecuted = true;
    expect(err).to.be.instanceOf(Error);
    expect(err?.message).to.equal("sync function error");
    expect(result).to.be.undefined;
    done();
  });

  // If error path is made async (per recommended refactor), add:
  // expect(callbackExecuted).to.be.false;
});

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b673581 and b8d6166.

📒 Files selected for processing (2)

• src/types.ts
• test/types-tests.spec.ts

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR `#506`, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.

Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR `#506`, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}