X11: Fix empty event race condition with a pipe

There is a seemingly unavoidable race condition when waiting for data on
the X11 display connection, as long as any other thread is also making
Xlib calls.  The event data we are waiting for could be read by the
other thread as part of looking for the reply to its request, before our
poll has begun.

This commit replaces the X11 event sent by glfwPostEmptyEvent with
writing to an unnamed pipe.  The race condition remains if other Xlib
calls are made on other threads, but glfwPostEmptyEvent should now be
race-free.

This commit is based on work by pcwalton, OlivierSohn, kovidgoyal and
joaodasilva.

Closes #2033
Related to #379
Related to #1281
Related to #1285

(cherry picked from commit cd22e28495)
This commit is contained in:
Camilla Löwy 2022-02-18 14:29:43 +01:00
parent e219c00d87
commit 5ccc756c56
6 changed files with 89 additions and 9 deletions

View File

@ -180,12 +180,14 @@ video tutorials.
- Ali Sherief - Ali Sherief
- Yoshiki Shibukawa - Yoshiki Shibukawa
- Dmitri Shuralyov - Dmitri Shuralyov
- Joao da Silva
- Daniel Sieger - Daniel Sieger
- Daniel Skorupski - Daniel Skorupski
- Bradley Smith - Bradley Smith
- Cliff Smolinsky - Cliff Smolinsky
- Patrick Snape - Patrick Snape
- Erlend Sogge Heggen - Erlend Sogge Heggen
- Olivier Sohn
- Julian Squires - Julian Squires
- Johannes Stein - Johannes Stein
- Pontus Stenetorp - Pontus Stenetorp

View File

@ -129,6 +129,8 @@ information on what to include when reporting a bug.
(#2024) (#2024)
- [X11] Bugfix: Joystick events could lead to busy-waiting (#1872) - [X11] Bugfix: Joystick events could lead to busy-waiting (#1872)
- [X11] Bugfix: `glfwWaitEvents*` did not continue for joystick events - [X11] Bugfix: `glfwWaitEvents*` did not continue for joystick events
- [X11] Bugfix: `glfwPostEmptyEvent` could be ignored due to race condition
(#379,#1281,#1285,#2033)
- [Wayland] Added support for key names via xkbcommon - [Wayland] Added support for key names via xkbcommon
- [Wayland] Bugfix: Key repeat could lead to a race condition (#1710) - [Wayland] Bugfix: Key repeat could lead to a race condition (#1710)
- [Wayland] Bugfix: Activating a window would emit two input focus events - [Wayland] Bugfix: Activating a window would emit two input focus events

View File

@ -306,6 +306,12 @@ GLFW_TRANSPARENT_FRAMEBUFFER on Windows 7 if DWM transparency is off
(the Transparency setting under Personalization > Window Color). (the Transparency setting under Personalization > Window Color).
@subsubsection emptyevents_33 Empty events on X11 no longer roundtrip to server
Starting with GLFW 3.3.7, events posted with @ref glfwPostEmptyEvent now use a separate
unnamed pipe instead of sending an X11 client event to the helper window.
@subsection deprecations_33 Deprecations in version 3.3 @subsection deprecations_33 Deprecations in version 3.3
@subsubsection charmods_callback_33 Character with modifiers callback @subsubsection charmods_callback_33 Character with modifiers callback

View File

@ -36,6 +36,9 @@
#include <limits.h> #include <limits.h>
#include <stdio.h> #include <stdio.h>
#include <locale.h> #include <locale.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
// Translate the X11 KeySyms for a key to a GLFW key code // Translate the X11 KeySyms for a key to a GLFW key code
@ -968,6 +971,37 @@ static Window createHelperWindow(void)
CWEventMask, &wa); CWEventMask, &wa);
} }
// Create the pipe for empty events without assumuing the OS has pipe2(2)
//
static GLFWbool createEmptyEventPipe(void)
{
if (pipe(_glfw.x11.emptyEventPipe) != 0)
{
_glfwInputError(GLFW_PLATFORM_ERROR,
"X11: Failed to create empty event pipe: %s",
strerror(errno));
return GLFW_FALSE;
}
for (int i = 0; i < 2; i++)
{
const int sf = fcntl(_glfw.x11.emptyEventPipe[i], F_GETFL, 0);
const int df = fcntl(_glfw.x11.emptyEventPipe[i], F_GETFD, 0);
if (sf == -1 || df == -1 ||
fcntl(_glfw.x11.emptyEventPipe[i], F_SETFL, sf | O_NONBLOCK) == -1 ||
fcntl(_glfw.x11.emptyEventPipe[i], F_SETFD, df | FD_CLOEXEC) == -1)
{
_glfwInputError(GLFW_PLATFORM_ERROR,
"X11: Failed to set flags for empty event pipe: %s",
strerror(errno));
return GLFW_FALSE;
}
}
return GLFW_TRUE;
}
// X error handler // X error handler
// //
static int errorHandler(Display *display, XErrorEvent* event) static int errorHandler(Display *display, XErrorEvent* event)
@ -1089,6 +1123,9 @@ int _glfwPlatformInit(void)
getSystemContentScale(&_glfw.x11.contentScaleX, &_glfw.x11.contentScaleY); getSystemContentScale(&_glfw.x11.contentScaleX, &_glfw.x11.contentScaleY);
if (!createEmptyEventPipe())
return GLFW_FALSE;
if (!initExtensions()) if (!initExtensions())
return GLFW_FALSE; return GLFW_FALSE;
@ -1206,6 +1243,12 @@ void _glfwPlatformTerminate(void)
#if defined(__linux__) #if defined(__linux__)
_glfwTerminateJoysticksLinux(); _glfwTerminateJoysticksLinux();
#endif #endif
if (_glfw.x11.emptyEventPipe[0] || _glfw.x11.emptyEventPipe[1])
{
close(_glfw.x11.emptyEventPipe[0]);
close(_glfw.x11.emptyEventPipe[1]);
}
} }
const char* _glfwPlatformGetVersionString(void) const char* _glfwPlatformGetVersionString(void)

View File

@ -238,6 +238,7 @@ typedef struct _GLFWlibraryX11
double restoreCursorPosX, restoreCursorPosY; double restoreCursorPosX, restoreCursorPosY;
// The window whose disabled cursor mode is active // The window whose disabled cursor mode is active
_GLFWwindow* disabledCursorWindow; _GLFWwindow* disabledCursorWindow;
int emptyEventPipe[2];
// Window manager atoms // Window manager atoms
Atom NET_SUPPORTED; Atom NET_SUPPORTED;

View File

@ -114,8 +114,12 @@ static GLFWbool waitForX11Event(double* timeout)
// //
static GLFWbool waitForAnyEvent(double* timeout) static GLFWbool waitForAnyEvent(double* timeout)
{ {
nfds_t count = 1; nfds_t count = 2;
struct pollfd fds[2] = { { ConnectionNumber(_glfw.x11.display), POLLIN } }; struct pollfd fds[3] =
{
{ ConnectionNumber(_glfw.x11.display), POLLIN },
{ _glfw.x11.emptyEventPipe[0], POLLIN }
};
#if defined(__linux__) #if defined(__linux__)
if (_glfw.linjs.inotify > 0) if (_glfw.linjs.inotify > 0)
@ -137,6 +141,32 @@ static GLFWbool waitForAnyEvent(double* timeout)
return GLFW_TRUE; return GLFW_TRUE;
} }
// Writes a byte to the empty event pipe
//
static void writeEmptyEvent(void)
{
for (;;)
{
const char byte = 0;
const int result = write(_glfw.x11.emptyEventPipe[1], &byte, 1);
if (result == 1 || (result == -1 && errno != EINTR))
break;
}
}
// Drains available data from the empty event pipe
//
static void drainEmptyEvents(void)
{
for (;;)
{
char dummy[64];
const int result = read(_glfw.x11.emptyEventPipe[0], dummy, sizeof(dummy));
if (result == -1 && errno != EINTR)
break;
}
}
// Waits until a VisibilityNotify event arrives for the specified window or the // Waits until a VisibilityNotify event arrives for the specified window or the
// timeout period elapses (ICCCM section 4.2.2) // timeout period elapses (ICCCM section 4.2.2)
// //
@ -2782,6 +2812,8 @@ GLFWbool _glfwPlatformRawMouseMotionSupported(void)
void _glfwPlatformPollEvents(void) void _glfwPlatformPollEvents(void)
{ {
drainEmptyEvents();
#if defined(__linux__) #if defined(__linux__)
_glfwDetectJoystickConnectionLinux(); _glfwDetectJoystickConnectionLinux();
#endif #endif
@ -2826,13 +2858,7 @@ void _glfwPlatformWaitEventsTimeout(double timeout)
void _glfwPlatformPostEmptyEvent(void) void _glfwPlatformPostEmptyEvent(void)
{ {
XEvent event = { ClientMessage }; writeEmptyEvent();
event.xclient.window = _glfw.x11.helperWindowHandle;
event.xclient.format = 32; // Data is 32-bit longs
event.xclient.message_type = _glfw.x11.NULL_;
XSendEvent(_glfw.x11.display, _glfw.x11.helperWindowHandle, False, 0, &event);
XFlush(_glfw.x11.display);
} }
void _glfwPlatformGetCursorPos(_GLFWwindow* window, double* xpos, double* ypos) void _glfwPlatformGetCursorPos(_GLFWwindow* window, double* xpos, double* ypos)