From 9312de623038c29048f48ca552517183aa61554e Mon Sep 17 00:00:00 2001 From: Shizuo Fujita Date: Fri, 31 Jul 2026 18:10:14 +0900 Subject: [PATCH] Fix stale GVL comments in Coolio_Loop_process_event The comments here say the Global VM Lock is not held, and build a whole explanation on top of that: the event is stashed rather than dispatched because "we can't rb_funcall() anything" until the GVL comes back. That stopped being true. cool.io patches ev_run() to release the GVL only around the blocking backend call (ev.c, rb_thread_call_without_gvl on ev_backend_poll); libev invokes the events it collected after that call has returned, so this function runs with the GVL held. The code was never unsafe -- xrealloc() of the event buffer and the watcher lookup both happen under the GVL -- but a reader who believes the comment would conclude the opposite, and would be wary of touching anything here. Describe what the code actually does instead: events are stashed and dispatched by Coolio_Loop_dispatch_events() once ev_loop() returns, and a watcher detached during that dispatch nils its own stashed entries, which the dispatch walk skips. Comments only, no behaviour change. Co-Authored-By: Claude Opus 5 --- ext/cool.io/loop.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/ext/cool.io/loop.c b/ext/cool.io/loop.c index 9e2e4fd..f12871e 100644 --- a/ext/cool.io/loop.c +++ b/ext/cool.io/loop.c @@ -110,8 +110,7 @@ void Coolio_Loop_process_event(VALUE watcher, int revents) struct Coolio_Loop *loop_data; struct Coolio_Watcher *watcher_data; - /* The Global VM lock isn't held right now, but hopefully - * we can still do this safely */ + /* The Global VM Lock is held here, see the explanation below */ watcher_data = Coolio_Watcher_ptr(watcher); if (watcher_data->enabled == 0) { @@ -126,13 +125,14 @@ void Coolio_Loop_process_event(VALUE watcher, int revents) * * Our call path up to here looks a little something like: * - * -> release GVL -> event syscall -> libev callback - * (GVL = Global VM Lock) ^^^ You are here + * -> release GVL -> event syscall -> reacquire GVL -> libev callback + * (GVL = Global VM Lock) ^^^ You are here * - * We released the GVL in the Coolio_Loop_run_once() function - * so other Ruby threads can run while we make a blocking - * system call (one of epoll, kqueue, port, poll, or select, - * depending on the platform). + * libev is patched (see ev.c) to release the GVL around the blocking + * system call (one of epoll, kqueue, port, poll, or select, depending + * on the platform) so other Ruby threads can run while we wait there. + * Only that call runs without the GVL: libev invokes the events it + * collected after the call has returned, so we hold the GVL here. * * More specifically, this is a libev callback abstraction * called from a real libev callback in every watcher, @@ -150,19 +150,19 @@ void Coolio_Loop_process_event(VALUE watcher, int revents) * event fired, why the hell is it telling the loop? Why * doesn't it just rb_funcall() the appropriate callback? * - * Well, the problem is the Global VM Lock isn't held right - * now, so we can't rb_funcall() anything. In order to get - * it back we have to: + * Because Ruby code doesn't run from inside libev's own event + * invocation. Instead: * - * stash event and return -> acquire GVL -> dispatch to Ruby + * stash event and return -> ev_loop() returns -> dispatch to Ruby * - * Which is kinda ugly and confusing, but still gives us + * Which is kinda ugly and confusing, but still gives us * an O(1) event loop whose heart is in the kernel itself. w00t! * * So, stash the event in the loop's data struct. When we return * the ev_loop() call being made in the Coolio_Loop_run_once_blocking() - * function below will also return, at which point the GVL is - * reacquired and we can call out to Ruby */ + * function below will also return, and Coolio_Loop_dispatch_events() + * walks what we stashed and calls out to Ruby. A watcher detached + * along the way nils its own stashed entries, which that walk skips */ /* Grow the event buffer if it's too small */ if(loop_data->events_received >= loop_data->eventbuf_size) {