Skip to content

Commit f41520f

Browse files
Watson1978claude
andauthored
Fix stale GVL comments in Coolio_Loop_process_event (#106)
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 <noreply@anthropic.com>
1 parent 557934f commit f41520f

1 file changed

Lines changed: 15 additions & 15 deletions

File tree

ext/cool.io/loop.c

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,7 @@ void Coolio_Loop_process_event(VALUE watcher, int revents)
110110
struct Coolio_Loop *loop_data;
111111
struct Coolio_Watcher *watcher_data;
112112

113-
/* The Global VM lock isn't held right now, but hopefully
114-
* we can still do this safely */
113+
/* The Global VM Lock is held here, see the explanation below */
115114
watcher_data = Coolio_Watcher_ptr(watcher);
116115

117116
if (watcher_data->enabled == 0) {
@@ -126,13 +125,14 @@ void Coolio_Loop_process_event(VALUE watcher, int revents)
126125
*
127126
* Our call path up to here looks a little something like:
128127
*
129-
* -> release GVL -> event syscall -> libev callback
130-
* (GVL = Global VM Lock) ^^^ You are here
128+
* -> release GVL -> event syscall -> reacquire GVL -> libev callback
129+
* (GVL = Global VM Lock) ^^^ You are here
131130
*
132-
* We released the GVL in the Coolio_Loop_run_once() function
133-
* so other Ruby threads can run while we make a blocking
134-
* system call (one of epoll, kqueue, port, poll, or select,
135-
* depending on the platform).
131+
* libev is patched (see ev.c) to release the GVL around the blocking
132+
* system call (one of epoll, kqueue, port, poll, or select, depending
133+
* on the platform) so other Ruby threads can run while we wait there.
134+
* Only that call runs without the GVL: libev invokes the events it
135+
* collected after the call has returned, so we hold the GVL here.
136136
*
137137
* More specifically, this is a libev callback abstraction
138138
* called from a real libev callback in every watcher,
@@ -150,19 +150,19 @@ void Coolio_Loop_process_event(VALUE watcher, int revents)
150150
* event fired, why the hell is it telling the loop? Why
151151
* doesn't it just rb_funcall() the appropriate callback?
152152
*
153-
* Well, the problem is the Global VM Lock isn't held right
154-
* now, so we can't rb_funcall() anything. In order to get
155-
* it back we have to:
153+
* Because Ruby code doesn't run from inside libev's own event
154+
* invocation. Instead:
156155
*
157-
* stash event and return -> acquire GVL -> dispatch to Ruby
156+
* stash event and return -> ev_loop() returns -> dispatch to Ruby
158157
*
159-
* Which is kinda ugly and confusing, but still gives us
158+
* Which is kinda ugly and confusing, but still gives us
160159
* an O(1) event loop whose heart is in the kernel itself. w00t!
161160
*
162161
* So, stash the event in the loop's data struct. When we return
163162
* the ev_loop() call being made in the Coolio_Loop_run_once_blocking()
164-
* function below will also return, at which point the GVL is
165-
* reacquired and we can call out to Ruby */
163+
* function below will also return, and Coolio_Loop_dispatch_events()
164+
* walks what we stashed and calls out to Ruby. A watcher detached
165+
* along the way nils its own stashed entries, which that walk skips */
166166

167167
/* Grow the event buffer if it's too small */
168168
if(loop_data->events_received >= loop_data->eventbuf_size) {

0 commit comments

Comments
 (0)