Skip to content

fix: the notification event function is the main thread's environment's, made once - #96

Merged
sidorares merged 2 commits into
mainfrom
claude/blissful-mcclintock-13704d
Oct 2, 2026
Merged

sidorares merged 2 commits into
mainfrom
claude/blissful-mcclintock-13704d

Conversation

@sidorares

Copy link
Copy Markdown
Contributor

The bug

src/notifications.mm's ProbeCenter() made the process-wide response
threadsafe function gEvents in whichever environment first loaded the
module
, behind a plain bool gProbed. Its callback CallJsEvent runs on
that environment's thread, and hands the response to EmitOrHold →
CALEmit, which in pump mode (channel not open) delivers only when
pthread_main_np() is true.

So if a Worker got to the addon first:

  • every notification response afterwards crossed to the worker's thread and
    was silently dropped there — with the main thread's
    setBackendEventCallback() listener installed the whole time;
  • once that worker ended, CALTsfn marked the function unusable and
    responses were dropped for good;
  • gHeld, the hold-and-replay list, is the main thread's (pump2 reads it),
    so the worker's EmitOrHold was also writing a std::vector the main
    thread reads unsynchronised;
  • gProbed itself was an unsynchronised bool read and written from any JS
    thread.

The fix

The event function is now the main thread's environment's, made the once —
the same shape InitCalendars already uses for the EventKit store's change
function (#68):

static std::atomic<CALTsfn*> gEvents{nullptr};
...
if (pthread_main_np() && !gEvents.load()) { ... }

QueueEvent drops a response while it is null: the main thread has not
loaded the module, so nothing there could be listening either.

The centre itself is unchanged in behaviour — NSBundle,
+currentNotificationCenter and the delegate are the process's and still go
up at the first load whichever environment's it is, because the delegate has
to be in place before initApp()'s finishLaunching. Its guard is now a
std::once_flag, which also publishes gCenter / gUnavailable /
gCategories to every later loader's thread; a verb only ever runs in an
environment that has loaded the module, so its read of gCenter is behind
that call_once.

Test

New test/notifications.js, driving the postNotificationResponse seam
(which needs no app bundle — nothing there touches the centre):

  1. a Worker loads the addon and posts a response before the main thread
    has required it at all
    , and is gone again;
  2. only then does the main thread require('..'), hold two responses with no
    listener, install one, and get them replayed in arrival order;
  3. then one event per response, userInfo round-tripped through JSON,
    userText, an absent actionId reading as 'default', and no duplicates.

It also covers the argument shapes (9 bad ones, each a TypeError naming the
verb) and the capability readback — including that the posting verbs throw
naming the reason in a bare node rather than dropping silently.

Against the previous build it fails deterministically at step 2:

notifications: the responses from before the listener should replay []

Registered in npm test and in CI, between screencolor.js and
threaded.js.

Verified

  • npm run build — clean, no warnings
  • npm test — exit 0, whole suite, screen unlocked (the GUI tests included)

One flaky-under-load note: test/threaded.js's round-trip assertion
(p95 < 2 ms locally) tripped at 6.4 ms when run immediately after a full
node-gyp rebuild; on an idle machine it is p95 ≈ 0.11 ms over five runs.
Not related to this change — that path touches no notification code.

No issue was filed for this; found by inspection of the notifications face
(originally #26).

…'s, made once — a worker that loads the module first no longer swallows every response

ProbeCenter made the process-wide response function (gEvents) in whichever
environment first loaded the module, behind a plain bool. Its callback runs
on that environment's thread, and in pump mode CALEmit delivers only when
pthread_main_np() holds — so with a Worker there first, every notification
response afterwards crossed to the worker's thread and was silently dropped,
even while the main thread had a backend callback installed, and once that
worker ended it was dropped for good. The held list it pushes into is the
main thread's too, so that was also a worker's thread writing a vector the
main thread's pump2 reads.

The function is now the main thread's environment's, made the once, the way
the calendar store's change function already is: std::atomic<CALTsfn*>,
created in InitNotifications when pthread_main_np(). QueueEvent drops a
response while it is null — the main thread has not loaded the module, so
nothing there could be listening either.

The centre itself stays as it was: NSBundle, +currentNotificationCenter and
the delegate are the process's and go up at the first load whichever
environment's it is, since the delegate has to be in place before
finishLaunching. Its guard is now a std::once_flag rather than a bool read
and written from any JS thread, which also publishes gCenter, gUnavailable
and gCategories to every later loader's thread — a verb only runs in an
environment that has loaded the module, so its read is behind that.

test/notifications.js drives the postNotificationResponse seam, which needs
no app bundle: a Worker loads the addon and posts a response before the main
thread has required it at all, and only then does the main thread hold,
replay and receive responses. Against the previous build it fails with every
response dropped ("the responses from before the listener should replay []").
It also covers the argument shapes, the capability readback and the
verbs that throw without a centre, and userInfo's JSON round trip.
@sidorares
sidorares merged commit 9c5010f into main Oct 2, 2026
4 checks passed
@sidorares
sidorares deleted the claude/blissful-mcclintock-13704d branch October 2, 2026 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant