Add off-main-thread SDL dispatch#59
Merged
Merged
Conversation
macOS requires SDL window and event-pump calls on thread that created
the windows (the first XOpenDisplay caller); an Xlib entry point that
touches SDL from a client worker thread crashes there.
Add runOnMainThread: it runs the work inline when already on the main
event thread, otherwise posts an SDL_USEREVENT and blocks until the main
thread runs it. The event-pipe wake is load-bearing here; SDL3's native
SDL_RunOnMainThread does not wake a main thread blocked in SDL_WaitEvent,
so it is not adopted, only left as a documented delegation seam.
Rework the XInitThreads display lock from a plain recursive mutex into a
handoff-aware recursive monitor. A worker that hands SDL work to the main
thread sheds the lock while it blocks and reacquires after; while a
handoff is pending only the main thread (to run the work) and the
shedding worker (to reacquire) may take the freed lock, so no third
thread interleaves the worker's XLockDisplay critical section. The gate
is folded into ownership under the monitor mutex, so there is no
check-then-act race. A single atomic mainEventThreadId is the
main-thread-ownership predicate.
Route the SDL-touching window entry points through the dispatch when
called off the main thread: X{Map,Unmap,Raise}Window, XStoreName,
X{Iconify,Configure,Move,Resize}Window (via configureWindowRouted, with
XMoveResizeWindow now one atomic configure), XReparentWindow, and
XMap/XUnmapSubwindows. requireMainEventThread aborts at the SDL sources
if a routed body ever reaches a worker thread instead of crashing
silently off-main.
convertEvent runs on whatever thread drained the SDL queue, so the
MAIN_DISPATCH branch re-queues the event (or drops the command to unblock
the worker if the re-queue fails) when a non-main thread drained it,
rather than running the thunk off-main. mainDispatchHandleEvent defers
a non-handoff-owner thunk while a handoff is pending so it does not
interleave the owner.
macOS requires every SDL window call on the thread that created the windows. The off-main dispatch primitive already funnels the direct window entry points there; this extends the same guarantee to the remaining SDL sites a client worker thread can reach through property and hint writes, plus display teardown. WM-state appliers now self-route: a void(Window) helper runWindowOpOnMain runs an applier on the main thread when called off it, and each applier passes itself so the routed re-entry runs its body on main. On the main thread the guard is one atomic load then a direct call, so the hot event-handling path is unchanged. Covers Motif hints, _NET_WM_STATE, resizable, icon, borderless, and transient/modal (SDL_SetWindowModalFor, reachable via WM_TRANSIENT_FOR and _NET_WM_STATE_MODAL). XSetWMNormalHints routes its size-constraint and resizable tail with a hints-carrying thunk; the hints pointer stays valid because the worker blocks until the main thread reads it. XCloseDisplay's destroyScreenWindow teardown routes the SDL_DestroyWindow work as well. Park non-owner dispatch commands on a main-thread list while a display lock handoff is pending, instead of re-queuing them onto the SDL event queue where the main loop would spin on the still-drainable event and, under priority inversion, starve the worker that must clear the handoff. Flush them via mainDispatchRunDeferred from the event-drain paths so a handoff that clears with no new event still drives them. Guard compatUnlockDisplay on ownership and positive depth, mirroring the EPERM the replaced recursive mutex returned, so a foreign-thread or over-unlock cannot decrement another thread's depth or underflow. Drop the unused Display parameter from applyTransientForRelationship so it fits the void(Window) self-route directly. Add off-thread test coverage: WM-state property writes and transient/modal pairing, XCloseDisplay teardown routing, and deferred-command flush driven by XNextEvent. The teardown test polls the SDL queue with SDL_PollEvent, which the SDL2 dlsym shim did not wrap, so add the thunk in this commit to keep the SDL2 build linking. It composes over the shim's own SDL_PumpEvents and SDL_PeepEvents to honor the inline-text side queue and pipe-drain model rather than dlsym'ing the host symbol, and treats a NULL event as a non-removing probe (SDL_PEEKEVENT) like real SDL; only a real pointer dequeues.
test_main_dispatch_deferred_xnext relied on a single blocking XNextEvent returning exactly the deferred ClientMessage. XNextEvent returns the first queued X event, and when one is already queued -- a stray ConfigureNotify left by an earlier test -- it need not drain the SDL queue that carries the MAIN_DISPATCH thunks. So the handoff owner's thunk never ran, the locked worker stayed parked in runOnMainThread holding the display-lock handoff, and the next test deadlocked acquiring that lock: an indefinite hang that CI cancelled only after ~18 minutes. Pump and drain the event loop until both dispatches have run and the ClientMessage arrives, bounded, and join only once the workers are confirmed unblocked, aborting with a diagnostic otherwise. Apply the same "pump until the worker finishes, handling every routed dispatch" shape to test_off_thread_close_display_teardown, which had the same one-dispatch-then-blind-join hazard.
The hang fix also changed test_off_thread_close_display_teardown to pump SDL events until the worker's XCloseDisplay had fully returned. But XCloseDisplay routes only destroyScreenWindow to the main thread; its remaining teardown (TTF_Quit, SDL_Quit) runs on the worker after that dispatch completes. Pumping SDL_PollEvent through the worker's SDL_Quit raced it on SDL2 internal state, and thread-sanitize flagged it (free in SDL_Quit vs strcmp in SDL_PollEvent). Revert to servicing just the one routed dispatch, then stop touching SDL and join, as the test did before: the worker reaches SDL_Quit only after the main thread has left the poll loop. That hardening was unnecessary anyway -- this test never hung (deferred_xnext did), so it needed no behavioral change.
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.
macOS requires SDL window and event-pump calls on thread that created the windows (the first XOpenDisplay caller); an Xlib entry point that touches SDL from a client worker thread crashes there.
Add runOnMainThread: it runs the work inline when already on the main event thread, otherwise posts an SDL_USEREVENT and blocks until the main thread runs it. The event-pipe wake is load-bearing here; SDL3's native SDL_RunOnMainThread does not wake a main thread blocked in SDL_WaitEvent, so it is not adopted, only left as a documented delegation seam.
Rework the XInitThreads display lock (src/display.c) from a plain recursive mutex into a handoff-aware recursive monitor. A worker that hands SDL work to the main thread sheds the lock while it blocks and reacquires after; while a handoff is pending only the main thread (to run the work) and the shedding worker (to reacquire) may take the freed lock, so no third thread interleaves the worker's XLockDisplay critical section. The gate is folded into ownership under the monitor mutex, so there is no check-then-act race. A single atomic mainEventThreadId is the main-thread-ownership predicate.
Route the SDL-touching window entry points through the dispatch when called off the main thread: X{Map,Unmap,Raise}Window, XStoreName, XIconifyWindow, XConfigureWindow / XMoveWindow / XResizeWindow (via configureWindowRouted, with XMoveResizeWindow now one atomic configure), XReparentWindow, and XMap/XUnmapSubwindows. requireMainEventThread aborts at the SDL sources if a routed body ever reaches a worker thread instead of crashing silently off-main.
convertEvent runs on whatever thread drained the SDL queue, so the MAIN_DISPATCH branch re-queues the event (or drops the command to unblock the worker if the re-queue fails) when a non-main thread drained it, rather than running the thunk off-main. mainDispatchHandleEvent defers a non-handoff-owner thunk while a handoff is pending so it does not interleave the owner.
Summary by cubic
Adds synchronous off-main-thread dispatch so all SDL window and event calls run on the main event thread to prevent macOS crashes. Also adds an SDL2
SDL_PollEventshim and test hardening.New Features
runOnMainThread+MAIN_DISPATCH_EVENT_CODE: post anSDL_USEREVENTand block; runs inline on main and wakes the loop.displayLockReleaseForHandoff,displayLockHandoffPending,displayLockReacquire.X{Map,Unmap,Raise}Window,XStoreName,XIconifyWindow,X{Configure,Move,Resize,MoveResize}Window(atomic),XReparentWindow,XMapSubwindows,XUnmapSubwindows,XDestroyWindow,XDestroySubwindows.runWindowOpOnMain: Motif hints,_NET_WM_STATE, resizable, icon, borderless, transient/modal;XSetWMNormalHintstail routed;XCloseDisplayteardown (destroyScreenWindow) runs on main.Bug Fixes
mainDispatchRunDeferred.convertEventre-queues when drained off-main to avoid running thunks on a worker.requireMainEventThread;compatUnlockDisplaychecks owner and positive depth.SDL_PollEventoverSDL_PumpEvents/SDL_PeepEventsto preserve side-queue semantics and fix SDL2 link failures.ClientMessagearrives; fix ThreadSanitizer race in off-threadXCloseDisplayby servicing only the routeddestroyScreenWindowthen joining.Written for commit 6544cde. Summary will update on new commits.