From 4c2757ff773b0e411a7c58874760440a15341852 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Fri, 13 Nov 2026 17:20:45 +0100 Subject: [PATCH 1/3] socket: `open` with a block must close, which is 27 of the hangs in #916 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TCPSocket.open, TCPServer.open and Socket.open were bare aliases for .new, so the block form closed nothing: the socket outlived the block and the peer waited for an EOF that never came. ⛔ THAT IS THE WHOLE net-http COLUMN OF #916. ruby/spec's library/net-http/http/fixtures/http_server.rb serves ONE client at a time and shuts down by opening a NEW connection to write an unterminated "CLOSE": def close TCPSocket.open(ip, port) { |socket| socket.write "CLOSE" } @thread.join end With that socket still open the server's `gets("\r\n\r\n")` never returns, the listen loop never comes back to accept, and @thread.join never completes. All 27 library/net-http files end on that join. Measured against the restored MRI oracle, the same ten-line reproduction: closed? after the block MRI true rbgo false the peer's gets MRI "CLOSE" rbgo blocks for ever join MRI :joined rbgo hangs and after this change rbgo prints the same seven lines as MRI, in the same order, and exits 0. ⛔ IT IS NOT THE GVL, which is what I expected to find. A goroutine dump of a hung get_spec.rb puts the server thread correctly INSIDE ioBlock with the GVL released (readUntil -> bufio.ReadByte -> net.(*conn).Read) and the main thread in threadJoin. The emulated GVL behaves; the socket was simply still open. Nor does Net::HTTP leak its own connection: the reproduction shows "peer closed" after the request under BOTH engines. HOW IT CLOSES, and why there is no fourth spelling of close: it sends the CLASS'S OWN `close` method, so TCPSocket, TCPServer and Socket each keep the one close they already had -- each guarded by `s.closed`, hence idempotent, so a block that closed the socket itself is not closed twice. The close runs in a defer, so an exception or a break leaves the socket closed as well: this is an ensure, not a tidy-up on the happy path. The test asserts the contract on the socket the block was handed rather than through a peer, so a regression FAILS it instead of HANGING it: the block's value comes back (not the socket), closed? is true after a normal return and after an exception, and `open` with no block still answers with an open socket exactly as .new does. Refs #931, #916 Co-Authored-By: Claude Opus 5 --- internal/vm/socket.go | 35 +++++++++++++++++++++++++-- internal/vm/socket_raw.go | 2 +- internal/vm/socket_test.go | 49 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/internal/vm/socket.go b/internal/vm/socket.go index 1b6a3974..5d0b920d 100644 --- a/internal/vm/socket.go +++ b/internal/vm/socket.go @@ -109,6 +109,37 @@ func (vm *VM) registerSocket() { // registerTCPSocket installs TCPSocket.new plus the connected-socket instance // surface (read/gets/readpartial/write/print/puts/< Date: Fri, 13 Nov 2026 20:22:01 +0100 Subject: [PATCH 2/3] =?UTF-8?q?socket:=20nonblock=3F=20on=20the=20socket?= =?UTF-8?q?=20classes=20=E2=80=94=20the=20vacuous=20pass=20the=20open=20fi?= =?UTF-8?q?x=20exposed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ruby/spec ratchet refused the previous commit with +27 examples overall and ONE file down: passing examples: 23699 (baseline 23672, +27) -- REGRESSED (1) -- core/io/nonblock_spec.rb: 3 passing, baseline 4 (-1) ⛔ The gate was right to refuse, and nothing had regressed. The example is TCPServer.open(0) { |socket| socket.nonblock?.should == true } and while `open` ignored its block that body NEVER RAN: the example passed with no assertion evaluated, and the baseline recorded that pass. Honouring the block made it run, and it raised NoMethodError — TCPServer#nonblock? does not exist. A test that had never run started running. Measured both ways rather than assumed, because the +27 includes other work merged into main today and the -1 could have been someone else's: core/io/nonblock_spec.rb, main ba89ba6f: pass=4 fail=0 error=0 core/io/nonblock_spec.rb, this branch: pass=3 fail=0 error=1 NoMethodError undefined method 'nonblock?' for an instance of TCPServer So it was mine. IO#nonblock? is installed on IO alone and reads an *IOObj field, which a socket is not. This installs `nonblock?` on the socket classes FROM THE SAME `io/nonblock` feature hook, so one place decides what that require adds and the two cannot drift; the socket classes exist from startup, so they are there when it fires. The answer is `true` because it is true here: Go puts every socket descriptor in non-blocking mode and polls it through the runtime, which is also why MRI reports true for a socket by default. ⛔ `nonblock=` is deliberately absent. The blocking mode of these descriptors belongs to the Go runtime, and a writer that accepted `false` and changed nothing would be a more expensive lie than a missing method. MRI does allow it; the divergence is stated here rather than papered over. The file is back to 4 passing — but the fourth example now RUNS and asserts, where on main it passes with nothing evaluated. Same number, different meaning. Refs #931 Co-Authored-By: Claude Opus 5 --- internal/vm/io.go | 9 ++++++++- internal/vm/socket.go | 37 +++++++++++++++++++++++++++++++++++++ internal/vm/socket_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/internal/vm/io.go b/internal/vm/io.go index b142aea4..8448297f 100644 --- a/internal/vm/io.go +++ b/internal/vm/io.go @@ -299,7 +299,14 @@ func (vm *VM) registerIO() { // before its require. The featureHooks map is already created by registerPrime, // which runs first. vm.featureHooks["fcntl"] = vm.installFcntl - vm.featureHooks["io/nonblock"] = func() { installIONonblock(cIO) } + // ⛔ The sockets are installed from the SAME hook, not from their own + // registration: one place decides what `require "io/nonblock"` adds, so the + // two cannot drift apart. The socket classes exist from startup, so they are + // already there when this fires. + vm.featureHooks["io/nonblock"] = func() { + installIONonblock(cIO) + vm.installSocketNonblock() + } // IO::WaitReadable / IO::WaitWritable (io.c Init_IO) are the marker modules a // non-blocking read or write raises with: IO::EAGAINWaitReadable is an // Errno::EAGAIN subclass that includes IO::WaitReadable, so `rescue diff --git a/internal/vm/socket.go b/internal/vm/socket.go index 5d0b920d..e46d2a96 100644 --- a/internal/vm/socket.go +++ b/internal/vm/socket.go @@ -109,6 +109,43 @@ func (vm *VM) registerSocket() { // registerTCPSocket installs TCPSocket.new plus the connected-socket instance // surface (read/gets/readpartial/write/print/puts/< Date: Fri, 13 Nov 2026 21:35:07 +0100 Subject: [PATCH 3/3] socket: write the class guard so it has no unreachable statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage ratchet named installSocketNonblock at 83.3%, newly below 100%. The uncovered statement was the `continue`: cls, ok := vm.consts[name].(*RClass) if !ok { continue // ← all six classes exist, so nothing reaches this } All six socket classes are registered at startup, so that arm is unreachable on this build and no test can honestly cover it. Writing the guard as if cls, ok := vm.consts[name].(*RClass); ok { … } keeps exactly the same protection — a name that is not a class is skipped rather than type-asserted into a panic — and has no statement on the false path to explain away. ⛔ The alternative was a test that deletes a class from vm.consts to drive the `continue`. That buys the percentage by asserting a state the VM never has, and leaves the next reader wondering which of the two is the lie. Removing the dead statement is cheaper and truer. Measured before pushing, with covratchet -require (#925), which is what that flag exists for after two commits in one day failed to move this same number: required 2 function(s), matched 2 record(s) in a profile of 7351 all required functions at 100% The ruby/spec ratchet is already green on this head — the nonblock commit settled core/io/nonblock_spec.rb — so this is the last red. Refs #931 Co-Authored-By: Claude Opus 5 --- internal/vm/socket.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/vm/socket.go b/internal/vm/socket.go index e46d2a96..3fb0fc0e 100644 --- a/internal/vm/socket.go +++ b/internal/vm/socket.go @@ -135,14 +135,18 @@ func (vm *VM) registerSocket() { // changed nothing would be a more expensive lie than a missing method. MRI does // allow it; that divergence is stated rather than papered over. func (vm *VM) installSocketNonblock() { + // ⛔ The guard is an `if ok`, not an `if !ok { continue }`, and the difference + // is not style. All six classes exist on this build, so the early-continue + // form left a statement no test could reach and the coverage ratchet named it + // (83.3%). Writing it this way keeps exactly the same protection — a name + // that is not a class is skipped rather than asserted into a panic — with no + // unreachable statement to explain away. for _, name := range []string{"TCPSocket", "TCPServer", "Socket", "UNIXSocket", "UNIXServer", "UDPSocket"} { - cls, ok := vm.consts[name].(*RClass) - if !ok { - continue + if cls, ok := vm.consts[name].(*RClass); ok { + cls.define("nonblock?", func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { + return object.Bool(true) + }) } - cls.define("nonblock?", func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { - return object.Bool(true) - }) } }