Fix stale GVL comments in Coolio_Loop_process_event - #106
Merged
Conversation
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>
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
1 participant
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.
The comments in
Coolio_Loop_process_event()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:ext/libev/ev.c:3777—rb_thread_call_without_gvl((void *)ev_backend_poll, ...), inside theCOOLIO PATCHERY HO!block, wraps the poll and nothing else.ext/libev/ev.c:3818—EV_INVOKE_PENDINGruns after that call has returned, i.e. with the GVL reacquired.So this function runs with the GVL held. The code was never unsafe — the
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.This replaces the wrong rationale with what the code actually does, rather than guessing at a new one: events are stashed and dispatched by
Coolio_Loop_dispatch_events()onceev_loop()returns, and a watcher detached during that dispatch nils its own stashed entries, which the dispatch walk skips (ext/cool.io/watcher.c,ext/cool.io/loop.c). The parts that were already correct — the O(1) explanation, the note about this being a libev callback abstraction — are kept as they were, voice included.Comments only.
rspec— 67 examples, 0 failures.🤖 Generated with Claude Code