Repository navigation
fix(client): write websocket frames from a writer thread, fixing the JRuby deadlock - #680
Merged
Merged
Conversation
This reverts commit 080aadc (#644). On JRuby IO#readpartial waits in a NIO select while holding the channel's blocking lock, and write_nonblock and IO.select need that same lock to switch the channel to nonblocking mode. With the reader thread parked in readpartial, the handshake write never returns and Ferrum::Browser.new hangs forever. Any write after that waits for the browser to send something first, so the approach can't work on JRuby at all. Conflicts with the logger rework (#658) are resolved by keeping its logging. The 0.19.0 changelog entry stays, the history of that release doesn't change. Fixes #676
Brings back the protection from #644 in a way that works on JRuby: a browser that stops reading its socket blocked the sending thread forever while it held the lock every other command needs to be sent. Callers and the driver now only encode frames and queue them; a writer thread is the only code writing the socket, with blocking writes, and the reader the only code reading it. No lock is held across socket I/O. Each queued frame carries its command's deadline and is dropped if that passes before it's written, so a command behind a large one fails after its own timeout. A chunk the browser takes no data of for protocol_timeout closes the connection, and the commands still waiting get DeadBrowserError with the reason. Restores the rest of #644 as well: the command timeout is passed down to the websocket, and a pending command whose send raised is forgotten.
… hand On JRuby the process never exited after the suite: Puma was only killed at exit, and killing doesn't interrupt its reactor's select. The websocket test proxy used IO.copy_stream from two threads on the same sockets, which hits the same JRuby lock as #676, so browsers behind it timed out.
Concurrent::Map is a Hash on CRuby but a hash-ordered Java map on JRuby, so pages, windows(:last) and workers came back in a different random order per process. Targets are kept in a Concurrent::Hash and iterated over a snapshot, so a target added by the event thread meanwhile can't break the iteration.
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.
Fixes #676.
Problem
#644 replaced the blocking websocket write with
write_nonblock+IO.select, so a browser that stops reading couldn't block the sending thread forever while it held the send lock. On JRuby that deadlocks:IO#readpartialwaits in a NIO select holding the channel's blocking lock, andwrite_nonblock/IO.selectneed the same lock to switch the channel to nonblocking mode. The reader thread is parked inreadpartialbefore the handshake is written, soFerrum::Browser.newhangs forever. It's not only the handshake: any later write waits until the browser happens to send something.Change
timeout:, not the large one's.protocol_timeoutcloses the connection. Commands still waiting getDeadBrowserErrorwith the reason. A shorter pause doesn't kill the connection anymore.Behavior changes
TimeoutErrorat that timeout. The frame can't be cut off midway, so it still goes out if the browser resumes, and the late response is ignored, as for any timed-out command.Client::WebSocket#send_messagereturns once the frame is queued.JRuby in CI
jrubywith Java 21.rbsis installed on MRI only, it doesn't build on JRuby.readpartial/writeinstead ofIO.copy_streamfrom two threads, which hit the same JRuby lock.browser.pages,windows(:last)andworkerscame back in random order on JRuby: targets were kept in aConcurrent::Map, which is an insertion-orderedHashon CRuby but a hash-ordered Java map on JRuby. They're kept in aConcurrent::Hashnow.