fix: make the CDP send path thread-safe#602
Open
ikraamg wants to merge 1 commit into
Open
Conversation
Ferrum::Client is shared across threads (contexts/pages are documented as usable concurrently), but the send path held no locks: - Client#next_command_id incremented @command_id with a non-atomic read-modify-write, so concurrent commands could mint the same id and cross-wire their responses in @pendings. On JRuby, 8 threads x 200k commands lose ~50k ids to collisions; MRI is only protected by the timing of GVL preemption. - WebSocket drove websocket-driver (which holds no locks) from multiple threads at once: command frames from callers, pong/close frames from the reader thread. Interleaved writes corrupt the frame stream and the affected commands hang until the client timeout. Command ids are now minted under a mutex and every driver interaction (start/text/parse/close) is serialized on one lock. Co-Authored-By: Claude Fable 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
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.
Ferrum::Client is used from multiple threads, but two parts of the send path have no locks:
This PR:
Script that shows the id collision, no Chrome needed:
On JRuby 10.1 this prints 1,536,642 unique ids (50,735 collided) on main, and 1,600,000 unique ids (0 collided) with this branch. On MRI it prints 0 collided on both, because the GVL does not currently switch threads inside the increment; that is interpreter scheduling, not something the code guarantees.
Also ran spec/browser_spec.rb and spec/context_spec.rb against real Chrome (green), and 16 threads x 500 rounds of 500KB page.evaluate on one shared browser: 28.3s on main, 25.1s with this branch, so the locks do not slow the send path down.