Skip to content

fix: make the CDP send path thread-safe#602

Open
ikraamg wants to merge 1 commit into
rubycdp:mainfrom
ikraamg:thread-safe-send-path
Open

fix: make the CDP send path thread-safe#602
ikraamg wants to merge 1 commit into
rubycdp:mainfrom
ikraamg:thread-safe-send-path

Conversation

@ikraamg

@ikraamg ikraamg commented Jul 10, 2026

Copy link
Copy Markdown

Ferrum::Client is used from multiple threads, but two parts of the send path have no locks:

  • Client#next_command_id increments @command_id with a plain read and write. Two threads can get the same command id, and then one command receives the other's response from pendings while the other waits until it times out.
  • Client::WebSocket calls websocket-driver from several threads at once: command frames from callers, pong and close replies from the reader thread. websocket-driver has no locks of its own, so concurrent calls can mix the bytes of different frames on the socket.

This PR:

  • makes the command id increment atomic with a mutex
  • serializes all driver calls (start, text, parse, close) on one mutex
  • adds unit specs for both races; they fail on main and pass on this branch

Script that shows the id collision, no Chrome needed:

require "ferrum"

client = Ferrum::Client.allocate
client.instance_variable_set(:@command_id, 0)
client.instance_variable_set(:@command_id_mutex, Mutex.new) # used by this branch, unused on main

ids = Queue.new
Array.new(8) { Thread.new { 200_000.times { ids << client.build_message("Page.enable", {})[:id] } } }.each(&:join)

seen = Hash.new(0)
seen[ids.pop] += 1 until ids.empty?
puts "#{RUBY_ENGINE}: #{seen.size} unique ids for 1,600,000 commands (#{seen.count { |_, n| n > 1 }} collided)"

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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant