Do proper wayland teardown cleaning up listeners and fds#2066
Open
3v1n0 wants to merge 7 commits intoValveSoftware:masterfrom
Open
Do proper wayland teardown cleaning up listeners and fds#20663v1n0 wants to merge 7 commits intoValveSoftware:masterfrom
3v1n0 wants to merge 7 commits intoValveSoftware:masterfrom
Conversation
If we do not do this newer wlroots would rightly assert every time that
a surface gets closed with:
gamescope: types/wlr_compositor.c:734:
surface_handle_resource_destroy:
Assertion `wl_list_empty(&surface->events.commit.listener_list)'
failed.
gamescope: types/wlr_compositor.c:737:
surface_handle_resource_destroy:
Assertion `wl_list_empty(&surface->events.destroy.listener_list)'
failed.
Prevents:
gamescope: xwayland/server.c:462: wlr_xwayland_server_destroy:
Assertion `wl_list_empty(&server->events.ready.listener_list)' failed
3v1n0
changed the title
Starting from wlroots 0.19 all these would trigger assertion errors
Destroy the event signals handlers on shutdown
We were not closing the KMS session on destruction since the DRM FD might be still being polled by the page-flip thread. Handle this gracefully now by: - Create an event-fd - Pass the event-fd to the thread - Make the thread to repeatedly and atomically check if it should exit When we're about to finish the drm backend: - Mark the thread as thread as completed - Write to the event-fd to signal the thread that we're done - Join the thread to wait its completion - Finally close KMS on wayland side and unset the drm fd This allows to finally close all the remaining resources or signal connections that we created on wayland side
3v1n0
force-pushed
the
disconnect-from-signals
branch
from
ed8b88b to
863aaca
Compare
4 tasks
emersion
reviewed
Feb 7, 2026
| struct wl_listener *listener = new wl_listener(); | ||
| listener->notify = kms_device_handle_change; | ||
| wl_signal_add( &wlserver.wlr.device->events.change, listener ); | ||
| wlserver.wlr.device_change_listener = new wl_listener(); |
Collaborator
There was a problem hiding this comment.
Orthogonal nit: would be slightly nicer to not use new here and just embed the listener in the parent struct. Just something I'm noticing when reading the code, not something that needs fixing in this PR.
emersion
reviewed
Feb 7, 2026
| std::thread flip_handler_thread( flip_handler_thread_run ); | ||
| flip_handler_thread.detach(); | ||
| // Create an eventfd to wake the flip handler poll for immediate exit. | ||
| g_page_flip_eventfd = eventfd( 0, EFD_NONBLOCK | EFD_CLOEXEC ); |
Collaborator
There was a problem hiding this comment.
Do we really need an eventfd here? Wouldn't a pipe be enough if we close the write end to wake up the pageflip thread and detect HANGUP?
emersion
reviewed
Feb 7, 2026
| // Create an eventfd to wake the flip handler poll for immediate exit. | ||
| g_page_flip_eventfd = eventfd( 0, EFD_NONBLOCK | EFD_CLOEXEC ); | ||
| if ( g_page_flip_eventfd == -1 ) { | ||
| drm_log.errorf_errno( "page-flip eventfd creation failed" ); |
Collaborator
There was a problem hiding this comment.
Can we just fail startup if we can't create the FD? Having to handle missing FD everywhere is painful and will likely just result in a deadlock.
emersion
reviewed
Feb 7, 2026
| drm_log.errorf_errno( "polling for DRM events failed" ); | ||
| break; | ||
| } | ||
| if ( ret == 0 ) { |
Collaborator
There was a problem hiding this comment.
This cannot happen, timeout is -1.
emersion
reviewed
Feb 7, 2026
|
|
||
| // Signal the page-flip handler thread to exit and join it so it won't be | ||
| // using the DRM fd while we clean it up. The thread polls with a | ||
| // timeout, so this join will return shortly. |
Collaborator
There was a problem hiding this comment.
The thread doesn't poll with a timeout AFAIU.
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
2 participants
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.
In debian we're already using wlroots-0.19, but with it every time a client window is closed or when gamescope itself is closed, we crash with an assertion error.
So remove event listeners (helping #1990) and close the drm FD, ensuring that the page-flip thread is completed.
See commits for details.