Skip to content

Log websocket responses before delivering messages - #603

Merged
route merged 1 commit into
rubycdp:mainfrom
kanutocd:fix/logger-response-race
Aug 8, 2026
Merged

Log websocket responses before delivering messages#603
route merged 1 commit into
rubycdp:mainfrom
kanutocd:fix/logger-response-race

Conversation

@kanutocd

@kanutocd kanutocd commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Log websocket responses before delivering them to waiting commands
  • Prevent callers from reading logger output before the command response has been written

Verification

  • bundle exec rspec spec/unit/browser_spec.rb
  • bundle exec rspec spec/browser_spec.rb:44
  • bundle exec rubocop lib/ferrum/client/web_socket.rb spec/unit/browser_spec.rb
  • bundle exec rake test

@kanutocd
kanutocd marked this pull request as draft July 13, 2026 23:59
@kanutocd
kanutocd marked this pull request as ready for review July 14, 2026 00:02
@kanutocd
kanutocd marked this pull request as draft July 14, 2026 00:16
@kanutocd
kanutocd marked this pull request as ready for review July 14, 2026 01:04
@route

route commented Jul 30, 2026

Copy link
Copy Markdown
Member

That's not a race per se, but ok! You are not moving a comment along

@kanutocd
kanutocd force-pushed the fix/logger-response-race branch from f23d942 to 2b36af4 Compare August 8, 2026 04:25
@kanutocd kanutocd changed the title Fix websocket logger response race Log websocket responses before delivering messages Aug 8, 2026
@kanutocd
kanutocd force-pushed the fix/logger-response-race branch from 2b36af4 to 49b08af Compare August 8, 2026 04:33
@kanutocd

kanutocd commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@route Thanks, that makes sense. I updated the PR 's title to avoid framing this as a race, and moved the comment together with the @messages.push(data) if data line so it still documents the code it refers to.

@route
route merged commit f9fea57 into rubycdp:main Aug 8, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants