Repository navigation
fix: the notification event function is the main thread's environment's, made once - #96
Merged
Merged
Conversation
…'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.
…ntock-13704d # Conflicts: # package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
src/notifications.mm'sProbeCenter()made the process-wide responsethreadsafe function
gEventsin whichever environment first loaded themodule, behind a plain
bool gProbed. Its callbackCallJsEventruns onthat environment's thread, and hands the response to
EmitOrHold→CALEmit, which in pump mode (channel not open) delivers only whenpthread_main_np()is true.So if a Worker got to the addon first:
was silently dropped there — with the main thread's
setBackendEventCallback()listener installed the whole time;CALTsfnmarked the function unusable andresponses were dropped for good;
gHeld, the hold-and-replay list, is the main thread's (pump2reads it),so the worker's
EmitOrHoldwas also writing astd::vectorthe mainthread reads unsynchronised;
gProbeditself was an unsynchronisedboolread and written from any JSthread.
The fix
The event function is now the main thread's environment's, made the once —
the same shape
InitCalendarsalready uses for the EventKit store's changefunction (#68):
QueueEventdrops a response while it is null: the main thread has notloaded the module, so nothing there could be listening either.
The centre itself is unchanged in behaviour —
NSBundle,+currentNotificationCenterand the delegate are the process's and still goup at the first load whichever environment's it is, because the delegate has
to be in place before
initApp()'sfinishLaunching. Its guard is now astd::once_flag, which also publishesgCenter/gUnavailable/gCategoriesto every later loader's thread; a verb only ever runs in anenvironment that has loaded the module, so its read of
gCenteris behindthat
call_once.Test
New
test/notifications.js, driving thepostNotificationResponseseam(which needs no app bundle — nothing there touches the centre):
Workerloads the addon and posts a response before the main threadhas required it at all, and is gone again;
require('..'), hold two responses with nolistener, install one, and get them replayed in arrival order;
userInforound-tripped through JSON,userText, an absentactionIdreading as'default', and no duplicates.It also covers the argument shapes (9 bad ones, each a
TypeErrornaming theverb) and the capability readback — including that the posting verbs throw
naming the reason in a bare
noderather than dropping silently.Against the previous build it fails deterministically at step 2:
Registered in
npm testand in CI, betweenscreencolor.jsandthreaded.js.Verified
npm run build— clean, no warningsnpm 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 mslocally) tripped at 6.4 ms when run immediately after a fullnode-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).