From 0d8b6714e39af81cbd6f4fbad500872a715dea24 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Tue, 3 Oct 2017 10:03:29 +0200 Subject: [PATCH] Move XCB event handling into xcb_prepare_cb. Previously, we used ev_check watchers, which are executed at the beginning of an event loop iteration. This was problematic if one of the handlers happened to fill the XCB event queue, e.g. by reading a reply from X11 and an event happened in the meantime. In that situation, we would hand control to the event loop, entirely ignoring the pending event. This would manifest itself as a 1-minute hang, reproducible (sometimes) in the i3 testsuite. issue #2790 describes an instance of this issue in i3bar, and we fixed that by changing the watcher priority to run last. Handling events in xcb_prepare_cb has the same effect, as ev_prepare watchers are run just before the event loop goes to sleep. --- i3bar/src/xcb.c | 28 ++++------------------------ src/floating.c | 16 +++++++++------- src/main.c | 38 ++++++++++++++++---------------------- src/restore_layout.c | 13 ++----------- 4 files changed, 31 insertions(+), 64 deletions(-) diff --git a/i3bar/src/xcb.c b/i3bar/src/xcb.c index dfa66ee2..98f0dcbe 100644 --- a/i3bar/src/xcb.c +++ b/i3bar/src/xcb.c @@ -83,7 +83,6 @@ int mod_pressed = 0; /* Event watchers, to interact with the user */ ev_prepare *xcb_prep; -ev_check *xcb_chk; ev_io *xcb_io; ev_io *xkb_io; @@ -1075,21 +1074,11 @@ static void handle_resize_request(xcb_resize_request_event_t *event) { } /* - * This function is called immediately before the main loop locks. We flush xcb - * then (and only then) + * This function is called immediately before the main loop locks. We check for + * events from X11, handle them, then flush our outgoing queue. * */ void xcb_prep_cb(struct ev_loop *loop, ev_prepare *watcher, int revents) { - xcb_flush(xcb_connection); -} - -/* - * This function is called immediately after the main loop locks, so when one - * of the watchers registered an event. - * We check whether an X-Event arrived and handle it. - * - */ -void xcb_chk_cb(struct ev_loop *loop, ev_check *watcher, int revents) { xcb_generic_event_t *event; if (xcb_connection_has_error(xcb_connection)) { @@ -1210,6 +1199,8 @@ void xcb_chk_cb(struct ev_loop *loop, ev_check *watcher, int revents) { } free(event); } + + xcb_flush(xcb_connection); } /* @@ -1267,21 +1258,12 @@ char *init_xcb_early() { /* The various watchers to communicate with xcb */ xcb_io = smalloc(sizeof(ev_io)); xcb_prep = smalloc(sizeof(ev_prepare)); - xcb_chk = smalloc(sizeof(ev_check)); ev_io_init(xcb_io, &xcb_io_cb, xcb_get_file_descriptor(xcb_connection), EV_READ); ev_prepare_init(xcb_prep, &xcb_prep_cb); - ev_check_init(xcb_chk, &xcb_chk_cb); - - /* Within an event loop iteration, run the xcb_chk watcher last: other - * watchers might call xcb_flush(), which, unexpectedly, can also read - * events into the queue (see _xcb_conn_wait). Hence, we need to drain xcb’s - * queue last, otherwise we risk dead-locking. */ - ev_set_priority(xcb_chk, EV_MINPRI); ev_io_start(main_loop, xcb_io); ev_prepare_start(main_loop, xcb_prep); - ev_check_start(main_loop, xcb_chk); /* Now we get the atoms and save them in a nice data structure */ get_atoms(); @@ -1535,11 +1517,9 @@ void clean_xcb(void) { xcb_aux_sync(xcb_connection); xcb_disconnect(xcb_connection); - ev_check_stop(main_loop, xcb_chk); ev_prepare_stop(main_loop, xcb_prep); ev_io_stop(main_loop, xcb_io); - FREE(xcb_chk); FREE(xcb_prep); FREE(xcb_io); } diff --git a/src/floating.c b/src/floating.c index 5f46dcf9..14988818 100644 --- a/src/floating.c +++ b/src/floating.c @@ -667,7 +667,7 @@ void floating_resize_window(Con *con, const bool proportional, /* Custom data structure used to track dragging-related events. */ struct drag_x11_cb { - ev_check check; + ev_prepare prepare; /* Whether this modal event loop should be exited and with which result. */ drag_result_t result; @@ -686,7 +686,7 @@ struct drag_x11_cb { const void *extra; }; -static void xcb_drag_check_cb(EV_P_ ev_check *w, int revents) { +static void xcb_drag_prepare_cb(EV_P_ ev_prepare *w, int revents) { struct drag_x11_cb *dragloop = (struct drag_x11_cb *)w->data; xcb_motion_notify_event_t *last_motion_notify = NULL; xcb_generic_event_t *event; @@ -765,6 +765,8 @@ static void xcb_drag_check_cb(EV_P_ ev_check *w, int revents) { dragloop->extra); } free(last_motion_notify); + + xcb_flush(conn); } /* @@ -831,18 +833,18 @@ drag_result_t drag_pointer(Con *con, const xcb_button_press_event_t *event, xcb_ .callback = callback, .extra = extra, }; - ev_check *check = &loop.check; + ev_prepare *prepare = &loop.prepare; if (con) loop.old_rect = con->rect; - ev_check_init(check, xcb_drag_check_cb); - check->data = &loop; + ev_prepare_init(prepare, xcb_drag_prepare_cb); + prepare->data = &loop; main_set_x11_cb(false); - ev_check_start(main_loop, check); + ev_prepare_start(main_loop, prepare); while (loop.result == DRAGGING) ev_run(main_loop, EVRUN_ONCE); - ev_check_stop(main_loop, check); + ev_prepare_stop(main_loop, prepare); main_set_x11_cb(true); xcb_ungrab_keyboard(conn, XCB_CURRENT_TIME); diff --git a/src/main.c b/src/main.c index 3087f111..d87b9a29 100644 --- a/src/main.c +++ b/src/main.c @@ -35,9 +35,9 @@ struct rlimit original_rlimit_core; /** The number of file descriptors passed via socket activation. */ int listen_fds; -/* We keep the xcb_check watcher around to be able to enable and disable it +/* We keep the xcb_prepare watcher around to be able to enable and disable it * temporarily for drag_pointer(). */ -static struct ev_check *xcb_check; +static struct ev_prepare *xcb_prepare; extern Con *focused; @@ -95,28 +95,23 @@ bool xkb_supported = true; bool force_xinerama = false; /* - * This callback is only a dummy, see xcb_prepare_cb and xcb_check_cb. + * This callback is only a dummy, see xcb_prepare_cb. * See also man libev(3): "ev_prepare" and "ev_check" - customise your event loop * */ static void xcb_got_event(EV_P_ struct ev_io *w, int revents) { - /* empty, because xcb_prepare_cb and xcb_check_cb are used */ + /* empty, because xcb_prepare_cb are used */ } /* - * Flush before blocking (and waiting for new events) + * Called just before the event loop sleeps. Ensures xcb’s incoming and outgoing + * queues are empty so that any activity will trigger another event loop + * iteration, and hence another xcb_prepare_cb invocation. * */ static void xcb_prepare_cb(EV_P_ ev_prepare *w, int revents) { - xcb_flush(conn); -} - -/* - * Instead of polling the X connection socket we leave this to - * xcb_poll_for_event() which knows better than we can ever know. - * - */ -static void xcb_check_cb(EV_P_ ev_check *w, int revents) { + /* Process all queued (and possibly new) events before the event loop + sleeps. */ xcb_generic_event_t *event; while ((event = xcb_poll_for_event(conn)) != NULL) { @@ -139,6 +134,9 @@ static void xcb_check_cb(EV_P_ ev_check *w, int revents) { free(event); } + + /* Flush all queued events to X11. */ + xcb_flush(conn); } /* @@ -150,12 +148,12 @@ static void xcb_check_cb(EV_P_ ev_check *w, int revents) { void main_set_x11_cb(bool enable) { DLOG("Setting main X11 callback to enabled=%d\n", enable); if (enable) { - ev_check_start(main_loop, xcb_check); + ev_prepare_start(main_loop, xcb_prepare); /* Trigger the watcher explicitly to handle all remaining X11 events. * drag_pointer()’s event handler exits in the middle of the loop. */ - ev_feed_event(main_loop, xcb_check, 0); + ev_feed_event(main_loop, xcb_prepare, 0); } else { - ev_check_stop(main_loop, xcb_check); + ev_prepare_stop(main_loop, xcb_prepare); } } @@ -781,15 +779,11 @@ int main(int argc, char *argv[]) { ewmh_update_desktop_viewport(); struct ev_io *xcb_watcher = scalloc(1, sizeof(struct ev_io)); - xcb_check = scalloc(1, sizeof(struct ev_check)); - struct ev_prepare *xcb_prepare = scalloc(1, sizeof(struct ev_prepare)); + xcb_prepare = scalloc(1, sizeof(struct ev_prepare)); ev_io_init(xcb_watcher, xcb_got_event, xcb_get_file_descriptor(conn), EV_READ); ev_io_start(main_loop, xcb_watcher); - ev_check_init(xcb_check, xcb_check_cb); - ev_check_start(main_loop, xcb_check); - ev_prepare_init(xcb_prepare, xcb_prepare_cb); ev_prepare_start(main_loop, xcb_prepare); diff --git a/src/restore_layout.c b/src/restore_layout.c index bf16c864..b99a50c1 100644 --- a/src/restore_layout.c +++ b/src/restore_layout.c @@ -39,7 +39,6 @@ static TAILQ_HEAD(state_head, placeholder_state) state_head = static xcb_connection_t *restore_conn; static struct ev_io *xcb_watcher; -static struct ev_check *xcb_check; static struct ev_prepare *xcb_prepare; static void restore_handle_event(int type, xcb_generic_event_t *event); @@ -49,10 +48,6 @@ static void restore_xcb_got_event(EV_P_ struct ev_io *w, int revents) { } static void restore_xcb_prepare_cb(EV_P_ ev_prepare *w, int revents) { - xcb_flush(restore_conn); -} - -static void restore_xcb_check_cb(EV_P_ ev_check *w, int revents) { xcb_generic_event_t *event; if (xcb_connection_has_error(restore_conn)) { @@ -77,6 +72,8 @@ static void restore_xcb_check_cb(EV_P_ ev_check *w, int revents) { free(event); } + + xcb_flush(restore_conn); } /* @@ -91,7 +88,6 @@ void restore_connect(void) { /* This is not the initial connect, but a reconnect, most likely * because our X11 connection was killed (e.g. by a user with xkill. */ ev_io_stop(main_loop, xcb_watcher); - ev_check_stop(main_loop, xcb_check); ev_prepare_stop(main_loop, xcb_prepare); placeholder_state *state; @@ -107,7 +103,6 @@ void restore_connect(void) { */ xcb_disconnect(restore_conn); free(xcb_watcher); - free(xcb_check); free(xcb_prepare); } @@ -124,15 +119,11 @@ void restore_connect(void) { } xcb_watcher = scalloc(1, sizeof(struct ev_io)); - xcb_check = scalloc(1, sizeof(struct ev_check)); xcb_prepare = scalloc(1, sizeof(struct ev_prepare)); ev_io_init(xcb_watcher, restore_xcb_got_event, xcb_get_file_descriptor(restore_conn), EV_READ); ev_io_start(main_loop, xcb_watcher); - ev_check_init(xcb_check, restore_xcb_check_cb); - ev_check_start(main_loop, xcb_check); - ev_prepare_init(xcb_prepare, restore_xcb_prepare_cb); ev_prepare_start(main_loop, xcb_prepare); } -- 2.39.5