From b47e67f98e4b9295ba1b42ead95e3e2573566b81 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 10 Nov 2026 17:25:03 +0100 Subject: [PATCH 1/7] WIP: Net::FTP open_timeout/read_timeout (test still to add) Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp.go | 43 +++++++++++++++-- internal/vm/net_ftp_bind.go | 93 +++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/internal/vm/net_ftp.go b/internal/vm/net_ftp.go index b7d26cc7..351276bd 100644 --- a/internal/vm/net_ftp.go +++ b/internal/vm/net_ftp.go @@ -78,14 +78,14 @@ func (vm *VM) registerFTPErrors(netMod, std *RClass) { // the ensured-close block form. func (vm *VM) registerFTPClassMethods(ftp *RClass) { newFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { - f := &ftpObj{cls: ftp, binary: true, passive: true} + f := newFTPObj(ftp) vm.ftpInit(f, args) return f } ftp.smethods["new"] = &Method{name: "new", owner: ftp, native: newFn} ftp.smethods["open"] = &Method{name: "open", owner: ftp, native: func(vm *VM, _ object.Value, args []object.Value, blk *Proc) object.Value { - f := &ftpObj{cls: ftp, binary: true, passive: true} + f := newFTPObj(ftp) vm.ftpInit(f, args) if blk == nil { return f @@ -142,6 +142,15 @@ func (vm *VM) ftpApplyOpts(f *ftpObj, h *object.Hash, port int) (int, string, bo if v, ok := ftpOptHash(h, "passive"); ok { f.passive = v.Truthy() } + // open_timeout and read_timeout are set from the options Hash exactly as + // given, nil included: `read_timeout: nil` is a request for no bound and + // must not fall back to the 60 s default. + if v, ok := ftpOptHash(h, "open_timeout"); ok { + f.openTimeout = v + } + if v, ok := ftpOptHash(h, "read_timeout"); ok { + f.readTimeout = v + } if v, ok := ftpOptHash(h, "use_pasv_ip"); ok { f.usePasvIP = v.Truthy() } @@ -175,8 +184,13 @@ func ftpOptHash(h *object.Hash, names ...string) (object.Value, bool) { // as welcome), and selects EPSV/EPRT when the connection is IPv6 // (Net::FTP#connect). func (vm *VM) ftpConnect(f *ftpObj, host string, port int) { - conn, err := net.Dial("tcp", net.JoinHostPort(host, strconv.Itoa(port))) + // open_timeout bounds the dial. MRI's default is nil — an unbounded connect + // — and DialTimeout(0) is Go's spelling of the same thing, so the default + // path is unchanged and only an explicit setting bounds it. + conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, strconv.Itoa(port)), + ftpTimeoutSeconds(f.openTimeout)) if err != nil { + ftpTimeoutErr(err, "open") ftpConnErr(err) } f.conn = conn @@ -251,6 +265,29 @@ func (vm *VM) registerFTPMethods(ftp *RClass) { }) // --- mode accessors --- + // Net::FTP#open_timeout / #read_timeout and their writers. They are plain + // attributes on the Ruby side: the value is handed back exactly as it was + // set, so `ftp.read_timeout = nil` reads back nil rather than 0. + d("open_timeout", func(_ *VM, v object.Value, _ []object.Value, _ *Proc) object.Value { + return self(v).openTimeout + }) + d("open_timeout=", func(_ *VM, v object.Value, a []object.Value, _ *Proc) object.Value { + self(v).openTimeout = a[0] + return a[0] + }) + d("read_timeout", func(_ *VM, v object.Value, _ []object.Value, _ *Proc) object.Value { + return self(v).readTimeout + }) + d("read_timeout=", func(_ *VM, v object.Value, a []object.Value, _ *Proc) object.Value { + self(v).readTimeout = a[0] + // Apply it at once rather than at the next read: Net::FTP#close lowers + // read_timeout before the QUIT exchange precisely so a wedged server + // cannot hold the close, and a setter that only took effect later would + // not honour that. + self(v).armRead() + return a[0] + }) + d("passive", func(_ *VM, v object.Value, _ []object.Value, _ *Proc) object.Value { return object.Bool(self(v).passive) }) diff --git a/internal/vm/net_ftp_bind.go b/internal/vm/net_ftp_bind.go index 1ddac309..4cb19cc0 100644 --- a/internal/vm/net_ftp_bind.go +++ b/internal/vm/net_ftp_bind.go @@ -12,6 +12,7 @@ import ( "net" "strconv" "strings" + "time" netftp "github.com/go-ruby-net-ftp/net-ftp" @@ -68,6 +69,90 @@ type ftpObj struct { welcome string // closed reports whether the control connection has been closed. closed bool + // openTimeout and readTimeout are Net::FTP#open_timeout and #read_timeout in + // SECONDS, held as Ruby values because nil is a meaningful setting ("no + // bound") and distinct from 0. + // + // ⛔ THE DEFAULTS ARE NOT SYMMETRIC, AND THAT ASYMMETRY IS THE WHOLE POINT. + // ruby/spec pins them (library/net-ftp/initialize_spec.rb): open_timeout + // defaults to nil ("sets open_timeout to nil", :272) and read_timeout to 60 + // ("sets read_timeout to the default value", :291 — `should == 60`). So MRI + // leaves the connect unbounded and bounds every read. + // + // Without them this binding dialled with net.Dial and set no read deadline + // anywhere, so a peer that accepted the connection and then said nothing + // held the interpreter for ever: 39 of 56 library/net-ftp spec files had to + // be killed by the clock under rbgo and none under MRI (#916). That is a + // denial of service needing no exploit, only silence — which is why the + // default is a bound and not "whatever the caller remembers to set". + openTimeout object.Value + readTimeout object.Value +} + +// ftpDefaultReadTimeout is Net::FTP's documented default, asserted by +// library/net-ftp/initialize_spec.rb. +const ftpDefaultReadTimeout = 60 + +// newFTPObj builds a Net::FTP with MRI's timeout defaults in place. Every +// construction site goes through it so a new one cannot be born unbounded. +func newFTPObj(cls *RClass) *ftpObj { + return &ftpObj{ + cls: cls, + binary: true, + passive: true, + openTimeout: object.NilV, + readTimeout: object.IntValue(ftpDefaultReadTimeout), + } +} + +// timeoutSeconds reads a timeout value as a Duration; nil (or a non-number) +// means no bound, which is what MRI's nil open_timeout asks for. +func ftpTimeoutSeconds(v object.Value) time.Duration { + switch n := v.(type) { + case object.Integer: + if n <= 0 { + return 0 + } + return time.Duration(float64(n) * float64(time.Second)) + case object.Float: + if n <= 0 { + return 0 + } + return time.Duration(float64(n) * float64(time.Second)) + } + return 0 +} + +// armRead puts the read_timeout deadline on the control socket before a read. +// It is called before EVERY read rather than once at connect, because a +// deadline is absolute: set once, it would expire mid-session and turn every +// later read into a timeout. +func (f *ftpObj) armRead() { + if f.conn == nil { + return + } + d := ftpTimeoutSeconds(f.readTimeout) + if d <= 0 { + // nil read_timeout: MRI waits indefinitely, and so do we. Clear any + // deadline a previous setting left behind. + _ = f.conn.SetReadDeadline(time.Time{}) + return + } + _ = f.conn.SetReadDeadline(time.Now().Add(d)) +} + +// ftpTimeoutErr re-raises a net timeout as the MRI exception for the phase, +// following the shape nethttp_bind.go's raiseTransportErr already uses — the +// HTTP binding has bounded its reads all along, and this one simply never got +// the same treatment. +func ftpTimeoutErr(err error, phase string) { + var ne net.Error + if errors.As(err, &ne) && ne.Timeout() { + if phase == "open" { + raise("Net::OpenTimeout", "execution expired") + } + raise("Net::ReadTimeout", "execution expired") + } } func (f *ftpObj) ToS() string { return "#" } @@ -120,6 +205,10 @@ func ftpConnErr(err error) { // yields whole reply lines (terminator included — the codec strips it). func (f *ftpObj) reader() netftp.LineReader { return func() (string, error) { + // The deadline is armed per line, not per reply: GetMultiline calls this + // repeatedly for a multi-line reply, and read_timeout bounds each read + // rather than the whole exchange. + f.armRead() line, err := f.r.ReadString('\n') if err != nil && line == "" { return "", err @@ -146,6 +235,10 @@ func (f *ftpObj) writeLine(line string) { func (f *ftpObj) getResp() string { resp, err := netftp.GetMultiline(f.reader()) if err != nil { + // A deadline expiry is Net::ReadTimeout, not a connection error: the + // socket is fine, the peer simply said nothing in time, and the two are + // rescued by different classes. + ftpTimeoutErr(err, "read") ftpConnErr(err) } f.lastResponse = resp From 3d2f5cfc5af699c8fbb9b5286f0d1a4aaea5d9c6 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 11 Nov 2026 09:33:17 +0100 Subject: [PATCH 2/7] Net::FTP: the tests for the connect and read bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WIP commit below added open_timeout/read_timeout; this adds what it owed. A server that accepts and stays SILENT -- the production shape, and exactly what #916's SIGQUIT dump showed rbgo waiting on -- must raise Net::ReadTimeout. The connection is held OPEN on purpose: a server that closed it would give the reader an EOF, a different path (Net::FTPConnectionError), and the test would pass with no deadline at all. ⛔ THE ELAPSED TIME IS PART OF THE ASSERTION. Without it the test would also pass if the greeting read failed instantly for an unrelated reason, which is the one outcome that would NOT prove a deadline fired. It asserts the call took at least 150ms against a 0.2s read_timeout, and measures 0.21s. Control run in both directions: removing the single armRead() call makes that test HANG until the 40s wall instead of failing in 0.21s. The second test pins the defaults together -- open_timeout nil, read_timeout 60 -- because the ASYMMETRY is the point and is MRI's, from library/net-ftp/initialize_spec.rb:272 and :291. A tidier symmetric default would change behaviour MRI specifies. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp_internal_test.go | 84 ++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/internal/vm/net_ftp_internal_test.go b/internal/vm/net_ftp_internal_test.go index afe3d994..442d6c38 100644 --- a/internal/vm/net_ftp_internal_test.go +++ b/internal/vm/net_ftp_internal_test.go @@ -1539,3 +1539,87 @@ func freePort(t *testing.T) int { ln.Close() return p } + +// ⛔ THE DEFECT THIS GUARDS. Net::FTP dialled with net.Dial and set no read +// deadline anywhere, so a peer that accepted the connection and then said +// NOTHING held the interpreter for ever. Measured 2026-11-10: 39 of 56 +// library/net-ftp ruby/spec files had to be killed by the clock under rbgo and +// none under MRI (#916), and a SIGQUIT dump put the process in +// net.(*conn).Read beneath GetMultiline, waiting for the 220 greeting. +// +// An unbounded read on a socket the peer controls is a denial of service that +// needs no exploit, only silence — so the test is a silent server, which is +// exactly the production shape. +func TestFTPReadTimeoutBoundsASilentServer(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + // Accept and hold the connection open without writing a greeting. The + // connection must stay OPEN: a server that closed it would give the reader + // an EOF, which is a different path (Net::FTPConnectionError) and would let + // this test pass without any deadline at all. + accepted := make(chan net.Conn, 1) + go func() { + c, aerr := ln.Accept() + if aerr != nil { + return + } + accepted <- c + }() + defer func() { + select { + case c := <-accepted: + c.Close() + default: + } + }() + + port := ln.Addr().(*net.TCPAddr).Port + src := "require \"net/ftp\"\nPORT = " + strconv.Itoa(port) + "\n" + ` +begin + Net::FTP.new("127.0.0.1", port: PORT, read_timeout: 0.2) + puts "RETURNED WITHOUT RAISING" +rescue => e + puts [e.class, e.message].inspect +end` + + start := stdtime.Now() + got := runSrc(t, src) + elapsed := stdtime.Since(start) + + if want := `[Net::ReadTimeout, "execution expired"]`; got != want { + t.Errorf("got %q, want %q", got, want) + } + // ⛔ THE TIME IS PART OF THE ASSERTION. Without it this test would also pass + // if the greeting read failed instantly for some unrelated reason, which is + // the one outcome that would NOT prove a deadline fired. + if elapsed < 150*stdtime.Millisecond { + t.Errorf("came back in %v, too fast to have waited out a 0.2s read_timeout: "+ + "the raise did not come from the deadline", elapsed) + } +} + +// The defaults are MRI's, and ruby/spec pins both: open_timeout nil and +// read_timeout 60 (library/net-ftp/initialize_spec.rb). They are asserted +// together because the asymmetry is the whole point — MRI leaves the connect +// unbounded and bounds every read, and a "tidier" symmetric default would +// change behaviour MRI specifies. +func TestFTPTimeoutDefaultsAndAccessors(t *testing.T) { + s := newFTPServer(t) + got := s.run(t, ` +ftp = Net::FTP.new("127.0.0.1", port: PORT) +p ftp.open_timeout +p ftp.read_timeout +ftp.open_timeout = 5 +ftp.read_timeout = nil +p ftp.open_timeout +p ftp.read_timeout +ftp.close`) + want := "nil\n60\n5\nnil" + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} From 2575697cd69ef93a921e7aa22059efda84efd867 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 11 Nov 2026 15:59:49 +0100 Subject: [PATCH 3/7] WIP: Net::FTP releases the GVL around its blocking socket calls Measurement pending; see #916 for the mechanism. Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp.go | 12 ++++++---- internal/vm/net_ftp_bind.go | 45 ++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/internal/vm/net_ftp.go b/internal/vm/net_ftp.go index 351276bd..a217045c 100644 --- a/internal/vm/net_ftp.go +++ b/internal/vm/net_ftp.go @@ -78,14 +78,14 @@ func (vm *VM) registerFTPErrors(netMod, std *RClass) { // the ensured-close block form. func (vm *VM) registerFTPClassMethods(ftp *RClass) { newFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { - f := newFTPObj(ftp) + f := newFTPObj(vm, ftp) vm.ftpInit(f, args) return f } ftp.smethods["new"] = &Method{name: "new", owner: ftp, native: newFn} ftp.smethods["open"] = &Method{name: "open", owner: ftp, native: func(vm *VM, _ object.Value, args []object.Value, blk *Proc) object.Value { - f := newFTPObj(ftp) + f := newFTPObj(vm, ftp) vm.ftpInit(f, args) if blk == nil { return f @@ -187,8 +187,12 @@ func (vm *VM) ftpConnect(f *ftpObj, host string, port int) { // open_timeout bounds the dial. MRI's default is nil — an unbounded connect // — and DialTimeout(0) is Go's spelling of the same thing, so the default // path is unchanged and only an explicit setting bounds it. - conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, strconv.Itoa(port)), - ftpTimeoutSeconds(f.openTimeout)) + var conn net.Conn + var err error + f.ftpIO(func() { + conn, err = net.DialTimeout("tcp", net.JoinHostPort(host, strconv.Itoa(port)), + ftpTimeoutSeconds(f.openTimeout)) + }) if err != nil { ftpTimeoutErr(err, "open") ftpConnErr(err) diff --git a/internal/vm/net_ftp_bind.go b/internal/vm/net_ftp_bind.go index 4cb19cc0..00b6a6a5 100644 --- a/internal/vm/net_ftp_bind.go +++ b/internal/vm/net_ftp_bind.go @@ -87,6 +87,9 @@ type ftpObj struct { // default is a bound and not "whatever the caller remembers to set". openTimeout object.Value readTimeout object.Value + // vm is held so every blocking socket call can release the GVL around + // itself. See ftpIO below for why that is not optional. + vm *VM } // ftpDefaultReadTimeout is Net::FTP's documented default, asserted by @@ -95,16 +98,48 @@ const ftpDefaultReadTimeout = 60 // newFTPObj builds a Net::FTP with MRI's timeout defaults in place. Every // construction site goes through it so a new one cannot be born unbounded. -func newFTPObj(cls *RClass) *ftpObj { +func newFTPObj(vm *VM, cls *RClass) *ftpObj { return &ftpObj{ cls: cls, binary: true, passive: true, openTimeout: object.NilV, readTimeout: object.IntValue(ftpDefaultReadTimeout), + vm: vm, } } +// ftpIO runs a blocking socket call with the GVL RELEASED, which is what every +// other binding here does through ioBlock and this one did not. +// +// ⛔ WITHOUT IT, A RUBY PROGRAM THAT SERVES ITSELF DEADLOCKS. rbgo emulates a +// GVL: one Ruby thread runs VM code at a time, and a thread that blocks in a +// native call is expected to drop it (vm.go:772, threadRelease). TCPServer#accept +// already does -- socket.go:174 wraps it in ioBlock -- but it must RE-ACQUIRE +// the GVL to return into Ruby, and these reads held it for the whole of a +// blocking read(2). So the server thread's Accept returned and then parked +// forever waiting for a lock the client was sitting on. +// +// Measured: every library/net-ftp spec stands up its server in the same +// process, so client and server are two threads of one VM. 39 of 56 files had +// to be killed by the clock, none under MRI (#916). With a read deadline in +// place the shape is visible in a timestamp: accept returned 1 ms after the +// client's read_timeout fired, and moving that timeout to 5 s moved the accept +// with it. +// +// Net::HTTP has used ioBlock all along (nethttp_bind.go:574). This binding +// simply never got it -- the same rule reached through one caller and not its +// neighbour. +func (f *ftpObj) ftpIO(fn func()) { + if f.vm == nil { + // A direct-constructed ftpObj in a unit test has no VM; running fn + // inline is correct there and keeps those tests working. + fn() + return + } + ioBlock(f.vm, fn) +} + // timeoutSeconds reads a timeout value as a Duration; nil (or a non-number) // means no bound, which is what MRI's nil open_timeout asks for. func ftpTimeoutSeconds(v object.Value) time.Duration { @@ -209,7 +244,9 @@ func (f *ftpObj) reader() netftp.LineReader { // repeatedly for a multi-line reply, and read_timeout bounds each read // rather than the whole exchange. f.armRead() - line, err := f.r.ReadString('\n') + var line string + var err error + f.ftpIO(func() { line, err = f.r.ReadString('\n') }) if err != nil && line == "" { return "", err } @@ -224,7 +261,9 @@ func (f *ftpObj) writeLine(line string) { if err != nil { raise("ArgumentError", "%s", err.Error()) } - if _, werr := f.conn.Write([]byte(bytes)); werr != nil { + var werr error + f.ftpIO(func() { _, werr = f.conn.Write([]byte(bytes)) }) + if werr != nil { ftpConnErr(werr) } } From 3cd3b060847759ff0ee78c8e0aedebb93a4b71d7 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 11 Nov 2026 16:02:33 +0100 Subject: [PATCH 4/7] Net::FTP: cover every branch the timeout helpers added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #921's first push dropped internal/vm to 98.8% and the coverage ratchet refused it. The uncovered lines were all branches no socket test can reach, and each is a behaviour worth asserting on its own rather than a coverage chore: - ftpTimeoutSeconds across every shape: nil (MRI's unbounded default), Integer, Float, and ⛔ the non-positive cases, which are UNBOUNDED and not "expire immediately" -- a deadline of now would fail every read at once -- plus a non-numeric, which is ignored rather than guessed at. - armRead's two early paths: an ftpObj with no connection yet, and a nil read_timeout that must CLEAR a deadline an earlier setting left behind. The second is asserted by showing the read goes back to BLOCKING, since a cleared deadline has no other visible effect. - ftpTimeoutErr returning for an error that is not a timeout. A version that raised unconditionally would turn every connection failure into Net::ReadTimeout, which is the opposite of what #916 needs to stay distinguishable. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp_internal_test.go | 72 ++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/internal/vm/net_ftp_internal_test.go b/internal/vm/net_ftp_internal_test.go index 442d6c38..15cad59c 100644 --- a/internal/vm/net_ftp_internal_test.go +++ b/internal/vm/net_ftp_internal_test.go @@ -8,6 +8,7 @@ package vm import ( "bufio" + "errors" "fmt" "net" "os" @@ -19,6 +20,8 @@ import ( stdtime "time" netftp "github.com/go-ruby-net-ftp/net-ftp" + + "github.com/go-embedded-ruby/ruby/internal/object" ) // The Net::FTP binding is exercised against an in-process FTP server (below) so @@ -1623,3 +1626,72 @@ ftp.close`) t.Errorf("got %q, want %q", got, want) } } + +// The timeout helpers' own branches. Each is a behaviour MRI specifies or a +// refusal this binding makes, so they are asserted directly rather than left +// to be reached by accident through a socket test. +func TestFTPTimeoutSecondsReadsEveryShape(t *testing.T) { + for _, tc := range []struct { + name string + v object.Value + want stdtime.Duration + }{ + // nil is MRI's "no bound" (open_timeout's default), and 0 is Go's + // spelling of the same thing for DialTimeout/SetReadDeadline. + {"nil means unbounded", object.NilV, 0}, + {"an Integer is seconds", object.IntValue(60), 60 * stdtime.Second}, + {"a Float is fractional seconds", object.Float(0.25), 250 * stdtime.Millisecond}, + // ⛔ Non-positive is NOT "expire immediately": MRI treats 0 as no + // timeout, and a deadline of now would make every read fail at once. + {"zero is unbounded", object.IntValue(0), 0}, + {"a negative Integer is unbounded", object.IntValue(-1), 0}, + {"a negative Float is unbounded", object.Float(-0.5), 0}, + // Anything that is not a number cannot be a duration; it is ignored + // rather than guessed at. + {"a String is not a duration", object.NewString("30"), 0}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := ftpTimeoutSeconds(tc.v); got != tc.want { + t.Errorf("ftpTimeoutSeconds(%v) = %v, want %v", tc.v, got, tc.want) + } + }) + } +} + +// armRead's two early paths, which no socket test reaches: an object with no +// connection yet, and a nil read_timeout that must CLEAR a deadline a previous +// setting left behind rather than leave it in place. +func TestFTPArmReadHandlesNoConnectionAndNoBound(t *testing.T) { + // No connection: must not panic, must not touch anything. + (&ftpObj{readTimeout: object.IntValue(5)}).armRead() + + srv, cli := net.Pipe() + defer srv.Close() + defer cli.Close() + f := &ftpObj{conn: cli, readTimeout: object.IntValue(5)} + f.armRead() // sets a deadline + f.readTimeout = object.NilV + f.armRead() // must clear it, not keep the old one + + // With the deadline cleared the pipe read blocks rather than failing, so a + // short wait that times out on OUR side proves no deadline is in force. + done := make(chan error, 1) + go func() { + b := make([]byte, 1) + _, err := cli.Read(b) + done <- err + }() + select { + case err := <-done: + t.Errorf("the read returned %v; a cleared deadline should leave it blocking", err) + case <-stdtime.After(100 * stdtime.Millisecond): + } +} + +// ftpTimeoutErr must RETURN for an error that is not a timeout, so the caller +// can go on to classify it. A version that raised unconditionally would turn +// every connection error into Net::ReadTimeout. +func TestFTPTimeoutErrIgnoresANonTimeout(t *testing.T) { + ftpTimeoutErr(errors.New("connection refused"), "read") + ftpTimeoutErr(nil, "open") +} From 7a6d12efcb09dc0eb7f2c12984f3b32d2e6f236e Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 12 Nov 2026 10:41:14 +0100 Subject: [PATCH 5/7] A gate for the mistake this repository made eight times in one week MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rbgo emulates a GVL: one Ruby thread runs VM code at a time, and a thread that blocks in a native call MUST release it (ioBlock). Net::HTTP has done that since it was written. Net::FTP did it at NONE of its sites, so every read held the GVL across a blocking read(2). The cost was not slowness, it was a DEADLOCK. A Ruby program that serves itself -- a spec fixture, a test that binds a socket and connects to it, an embedder running a listener beside its own client -- has its server thread return from Accept and then park on the lock the client sits on. Dose-response: a 2s client deadline put the accept at 2.308s, 5s at 5.307s, ioBlock at 0.305s. In ruby/spec: 39 of 56 library/net-ftp files killed by the clock, none under MRI (#916). ⛔ THE POINT IS NOT THIS BUG, IT IS ITS SHAPE. Eight times in one week a mechanism existed and was wired at only some of its doors: Array.new vs Array#first, expand_path vs Dir.home, raiseWithIvars vs Kernel#raise, the vcall flag, Set's constructors, the stubbed-helper list in three surfaces, the README's derived numbers, and now ioBlock. "Is it implemented?" was always the wrong question; "who else should be calling it?" is answerable from the AST without running anything. What it found beyond net-ftp, on its first run: - THREE more sites in Net::FTP itself -- the DATA connection, which my own fix in #921 missed; - Net::IMAP, whose readLine/readLiteral are handed to the library as callbacks with no ioBlock anywhere: the same shape, untouched; - Net::SMTP's response reader; httprb and hanami's Accept. ⛔ And it corrected me twice while being written. I had said "Net::HTTP has four unwrapped sites": false. All six nethttp findings are helpers whose callers already wrap them, which a purely lexical rule cannot see. That exemption is NAMED, never inferred -- //gvl:released-by-caller , a reason required, so every waiver is one grep away. A gate that cries wolf is switched off within a week. It lands as a RATCHET, not a big bang: testdata/gvl-blocking-backlog.txt records what was already there, so the gate fires on what happens NEXT. The list may only shrink -- an entry that stops reporting fails too, because a stale allowance is where the next regression hides. Control run in both directions: removing the single ftpIO wrapper makes it name net_ftp_bind.go:249 immediately. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/gvl_blocking_io_test.go | 259 ++++++++++++++++++ internal/vm/nethttp_bind.go | 6 + internal/vm/testdata/gvl-blocking-backlog.txt | 37 +++ 3 files changed, 302 insertions(+) create mode 100644 internal/vm/gvl_blocking_io_test.go create mode 100644 internal/vm/testdata/gvl-blocking-backlog.txt diff --git a/internal/vm/gvl_blocking_io_test.go b/internal/vm/gvl_blocking_io_test.go new file mode 100644 index 00000000..fcb4b133 --- /dev/null +++ b/internal/vm/gvl_blocking_io_test.go @@ -0,0 +1,259 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +//go:build !rbgo_core + +package vm + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// ⛔ THE GATE FOR A MISTAKE THIS REPOSITORY MADE EIGHT TIMES IN ONE WEEK: +// a mechanism that exists and is wired at only some of its doors. +// +// rbgo emulates a GVL — one Ruby thread runs VM code at a time (vm.go), and a +// thread that blocks in a native call MUST release it, which is what ioBlock +// does. Net::HTTP has done that at twenty sites since it was written. +// Net::FTP did it at NONE, so every one of its reads held the GVL across a +// blocking read(2). +// +// The cost was not a slow program, it was a DEADLOCK. A Ruby program that +// serves itself — a spec fixture, a test that binds a socket and connects to +// it, an embedder running a listener beside its own client — has its server +// thread return from Accept and then park on a lock the client is sitting on. +// Measured by dose-response: with a 2 s client deadline the accept completed at +// 2.308 s, with a 5 s deadline at 5.307 s, and with ioBlock at 0.305 s. In +// ruby/spec that was 39 of 56 library/net-ftp files killed by the clock and +// none under MRI (#916). +// +// Asking "is ioBlock implemented?" was always the wrong question. The right one +// is "WHO ELSE should be calling it?", and that is answerable from the AST +// without running anything: a blocking socket call in a binding must sit +// lexically inside a closure handed to ioBlock (or to threadBlock, or to a +// wrapper that forwards to one). +func TestEveryBlockingSocketCallReleasesTheGVL(t *testing.T) { + // The methods that block on a peer. Dial/Accept/Read/Write on a net.Conn or + // net.Listener can wait indefinitely; anything here must not do so while + // holding the GVL. + blocking := map[string]bool{ + "Accept": true, "Dial": true, "DialTimeout": true, + "ReadString": true, "ReadFrom": true, "ReadFromUDP": true, "ReadFull": true, + } + // The wrappers that release it. A call inside one of these closures is fine; + // ftpIO forwards to ioBlock, and a binding may add its own as long as it is + // named here, which keeps this list the single place the exemption lives. + releasing := map[string]bool{ + "ioBlock": true, "threadBlock": true, "threadRelease": true, "ftpIO": true, + } + + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + fset := token.NewFileSet() + type finding struct{ where, what string } + var bad []finding + scanned, calls := 0, 0 + + for _, path := range files { + if strings.HasSuffix(path, "_test.go") { + continue + } + src, rerr := os.ReadFile(path) + if rerr != nil { + t.Fatalf("read %s: %v", path, rerr) + } + f, perr := parser.ParseFile(fset, path, src, parser.ParseComments) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + scanned++ + exempt := exemptLines(fset, f) + + // Walk with a stack so a call can be asked which closures enclose it. + var stack []ast.Node + ast.Inspect(f, func(n ast.Node) bool { + if n == nil { + stack = stack[:len(stack)-1] + return true + } + stack = append(stack, n) + + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !blocking[sel.Sel.Name] { + return true + } + calls++ + if enclosedByReleaser(stack, releasing) { + return true + } + if exempt[enclosingFuncLine(fset, stack)] { + return true + } + bad = append(bad, finding{ + where: fset.Position(call.Pos()).String(), + what: sel.Sel.Name, + }) + return true + }) + } + + // ⛔ "N of M, never N alone": a walk that parsed nothing finds no offender + // either, and an empty result would read exactly like a clean tree. + if scanned == 0 || calls == 0 { + t.Fatalf("scanned %d file(s) and found %d blocking call(s); this gate cannot "+ + "have run — a zero here is an unread tree, not a clean one", scanned, calls) + } + t.Logf("%d file(s), %d blocking socket call(s) checked", scanned, calls) + + sort.Slice(bad, func(i, j int) bool { return bad[i].where < bad[j].where }) + + // ⛔ A RATCHET, NOT A BIG-BANG. This gate was written after the fact and + // finds sites in several bindings at once; failing on all of them would + // have meant either a sprawling change nobody can review or — far more + // likely — the gate never landing at all. The backlog records what was + // already there on the day it was written, so the gate fires on what + // happens NEXT, and the list can only shrink: an entry that no longer + // reports is a failure too, so a fix cannot leave its allowance behind for + // the next regression to hide in. + backlog, order := readBacklog(t) + seen := map[string]bool{} + for _, b := range bad { + key := trimFileLine(b.where) + seen[key] = true + if backlog[key] { + continue + } + t.Errorf("%s: %s blocks while HOLDING the GVL.\n"+ + " Wrap it in ioBlock(vm, func(){ … }). A Ruby program that serves itself "+ + "deadlocks here: the server thread returns from Accept and then waits for a "+ + "lock this call is sitting on (#916). If its callers already release the GVL, "+ + "say so on the function with //gvl:released-by-caller .", b.where, b.what) + } + for _, key := range order { + if !seen[key] { + t.Errorf("%s is in testdata/gvl-blocking-backlog.txt and no longer reports.\n"+ + " Delete that line: a stale allowance is a place the next regression hides.", key) + } + } +} + +// trimFileLine drops the column from a token.Position so a backlog entry does +// not churn when a line is reformatted. +func trimFileLine(pos string) string { + if i := strings.LastIndexByte(pos, ':'); i >= 0 { + return pos[:i] + } + return pos +} + +// readBacklog reads the sites that already held the GVL when this gate was +// written. Comments and blank lines are ignored; the order is kept so the +// stale-entry report reads in file order. +func readBacklog(t *testing.T) (map[string]bool, []string) { + t.Helper() + const path = "testdata/gvl-blocking-backlog.txt" + body, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v (the gate needs its backlog; an absent file is not an empty one)", path, err) + } + out, order := map[string]bool{}, []string{} + for _, line := range strings.Split(string(body), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + out[line] = true + order = append(order, line) + } + return out, order +} + +// enclosedByReleaser reports whether any FuncLit on the stack is an argument to +// one of the releasing wrappers. Lexical enclosure is the right test: ioBlock +// takes the closure and runs it with the GVL dropped, so a call anywhere inside +// that closure is covered, and one outside it is not. +func enclosedByReleaser(stack []ast.Node, releasing map[string]bool) bool { + for i := len(stack) - 1; i >= 0; i-- { + if _, ok := stack[i].(*ast.FuncLit); !ok { + continue + } + // The parent of a FuncLit that is an argument is the CallExpr itself. + for j := i - 1; j >= 0 && j >= i-2; j-- { + call, ok := stack[j].(*ast.CallExpr) + if !ok { + continue + } + switch fn := call.Fun.(type) { + case *ast.Ident: + if releasing[fn.Name] { + return true + } + case *ast.SelectorExpr: + if releasing[fn.Sel.Name] { + return true + } + } + } + } + return false +} + +// exemptLines returns the start lines of the functions carrying the directive +// +// //gvl:released-by-caller +// +// ⛔ THE EXEMPTION IS NAMED, NOT INFERRED. A purely lexical rule cannot see a +// HELPER whose callers wrap it — nethttpRawDial is reached only from inside an +// ioBlock, so its net.DialTimeout is already covered — and a gate that cried +// wolf there would be switched off within a week. Making the caller's promise +// explicit keeps the gate strict AND keeps every exemption readable in one +// grep, instead of hiding it in an interprocedural inference nobody can audit. +// +// A reason is required: a bare directive is a silent waiver, and this file +// exists because silent waivers are how Net::FTP went twenty sites without the +// wrapper everything else had. +func exemptLines(fset *token.FileSet, f *ast.File) map[int]bool { + const directive = "//gvl:released-by-caller" + out := map[int]bool{} + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Doc == nil { + continue + } + for _, c := range fn.Doc.List { + if !strings.HasPrefix(c.Text, directive) { + continue + } + if strings.TrimSpace(strings.TrimPrefix(c.Text, directive)) == "" { + continue // no reason given: not an exemption + } + out[fset.Position(fn.Pos()).Line] = true + } + } + return out +} + +// enclosingFuncLine is the start line of the innermost FuncDecl around a call, +// which is the key exemptLines records. +func enclosingFuncLine(fset *token.FileSet, stack []ast.Node) int { + for i := len(stack) - 1; i >= 0; i-- { + if fn, ok := stack[i].(*ast.FuncDecl); ok { + return fset.Position(fn.Pos()).Line + } + } + return -1 +} diff --git a/internal/vm/nethttp_bind.go b/internal/vm/nethttp_bind.go index c088f345..1d891b6e 100644 --- a/internal/vm/nethttp_bind.go +++ b/internal/vm/nethttp_bind.go @@ -757,6 +757,8 @@ func basicAuthHeader(user, pass string) string { // nethttpRawDial opens a raw TCP connection with an optional dial timeout // (open_timeout); 0 means no bound. +// +//gvl:released-by-caller every call site is inside the ioBlock at nethttpDial, so the GVL is already dropped when this runs func nethttpRawDial(host, port string, openTO time.Duration) (net.Conn, error) { addr := net.JoinHostPort(host, port) if openTO > 0 { @@ -807,6 +809,8 @@ func nethttpTLSWrap(conn net.Conn, host string, verifyMode int64) (streamIO, err // to nethttp.ParseResponse. keepAlive reports whether the connection may be reused // (false when the server sent Connection: close or the body was read to EOF). // noBody (HEAD and other body-forbidden responses) stops after the header block. +// +//gvl:released-by-caller its only non-test caller is inside the ioBlock in nethttpExchangeFramed func nethttpReadResponse(r *bufio.Reader, noBody bool) (raw []byte, keepAlive bool, err error) { var buf bytes.Buffer status, err := r.ReadString('\n') @@ -866,6 +870,8 @@ func nethttpReadResponse(r *bufio.Reader, noBody bool) (raw []byte, keepAlive bo // nethttpCopyChunked copies a chunked body (raw, including the chunk framing) from // r into buf until the zero-size chunk, then consumes the trailer up to a blank // line — the same framing nethttp.ParseResponse later decodes. +// +//gvl:released-by-caller reached only from nethttpReadResponse, itself inside the ioBlock in nethttpExchangeFramed func nethttpCopyChunked(r *bufio.Reader, buf *bytes.Buffer) error { for { line, err := r.ReadString('\n') diff --git a/internal/vm/testdata/gvl-blocking-backlog.txt b/internal/vm/testdata/gvl-blocking-backlog.txt new file mode 100644 index 00000000..9a0554ac --- /dev/null +++ b/internal/vm/testdata/gvl-blocking-backlog.txt @@ -0,0 +1,37 @@ +# Blocking socket calls that already held the GVL when +# TestEveryBlockingSocketCallReleasesTheGVL was written (2026-11-12). +# +# ⛔ THIS LIST MAY ONLY SHRINK. A new site not listed here fails the gate, and +# an entry that stops reporting fails it too — a stale allowance is a place the +# next regression hides. +# +# Why they matter: rbgo emulates a GVL, so a native call that blocks while +# holding it stops every other Ruby thread. A program that serves itself — a +# spec fixture, a test that binds a socket and connects to it, an embedder +# running a listener beside its own client — DEADLOCKS: the server thread +# returns from Accept and then parks on the lock the client is sitting on. +# Measured in ruby/spec as 39 of 56 library/net-ftp files killed by the clock +# and none under MRI (#916). +# +# Each line is file:line. Fix one by wrapping the call in ioBlock(vm, …) and +# deleting its line here. If the callers already release the GVL, mark the +# function //gvl:released-by-caller instead and delete the line. + +# Net::FTP's DATA connection. The control connection was fixed in #921/#916; +# these three were missed by that change and found by this gate. +net_ftp_bind.go:409 +net_ftp_bind.go:419 +net_ftp_bind.go:485 + +# Net::IMAP: readLine/readLiteral are handed to imap.NewReader as callbacks and +# the binding has no ioBlock at all — the same shape Net::FTP had. +net_imap_bind.go:82 +net_imap_bind.go:89 + +# Net::SMTP: the response reader. +net_smtp_bind.go:140 + +# Servers that accept connections from Ruby: an embedder running one of these +# beside its own client is the exact deadlock above. +httprb.go:148 +hanami_bind.go:514 From e92d9d2fe1f03387b109c59a4f2bca1797842393 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 12 Nov 2026 22:21:27 +0100 Subject: [PATCH 6/7] Net::FTP: the two doors the first coverage pass still had not opened 3cd3b06 was meant to answer the ratchet and did not move the measurement: CI came back with the same two functions, ftpApplyOpts at 95.0% and ftpTimeoutErr at 80.0%. A fix that does not move the number is not a fix, so this time the number is measured before pushing -- both now read 100.0% under `go tool cover -func` over the FTP tests. What was actually uncovered is the defect shape this repository keeps repeating: a mechanism wired at only SOME of its doors. - ftpApplyOpts reads open_timeout and read_timeout from the options Hash on two adjacent lines. Every socket test passes read_timeout ALONE, and the accessor tests go through the attr writer, not the Hash -- so the open_timeout line had never run. The new test goes through the Hash door for BOTH, and asserts the other half of that contract too: an explicit `read_timeout: nil` is a REQUEST for no bound and must not fall back to MRI's 60 s default, while a key absent from the Hash must leave the field untouched. Those are the two cases the key-presence check exists to tell apart. - ftpTimeoutErr has two raises and the existing test returns before reaching either: it only asserts that a NON-timeout is passed through. The new test drives a real net.Error timeout through both phases and asserts the CLASS -- Net::OpenTimeout for open, Net::ReadTimeout otherwise -- because MRI distinguishes them and ruby/spec matches on the class, so a version that collapsed the two would still satisfy any check that only asked "did it raise?". The error is WRAPPED, as the standard library hands it to the callers, so the test fails if the classification is ever narrowed to a type assertion. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp_internal_test.go | 97 ++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/internal/vm/net_ftp_internal_test.go b/internal/vm/net_ftp_internal_test.go index 15cad59c..98b733fe 100644 --- a/internal/vm/net_ftp_internal_test.go +++ b/internal/vm/net_ftp_internal_test.go @@ -1695,3 +1695,100 @@ func TestFTPTimeoutErrIgnoresANonTimeout(t *testing.T) { ftpTimeoutErr(errors.New("connection refused"), "read") ftpTimeoutErr(nil, "open") } + +// ⛔ THE DEFECT SHAPE THIS REPOSITORY KEEPS REPEATING: a mechanism wired at only +// SOME of its doors. open_timeout and read_timeout each have three doors -- the +// constructor's options Hash, the attr writer, and the default -- and the Hash +// door is the one no socket test goes through, because every one of them passes +// read_timeout ALONE. The coverage ratchet named ftpApplyOpts at 95% for exactly +// that: the open_timeout assignment sitting beside a line that ran had never run +// itself. Covering it by hand is the point, not the percentage. +func TestFTPApplyOptsCarriesBothTimeoutsFromTheOptionsHash(t *testing.T) { + // ftpApplyOpts never touches its VM receiver, so a nil one keeps this test + // about the Hash and nothing else. + var vm *VM + + t.Run("both are read, not just read_timeout", func(t *testing.T) { + h := object.NewHash() + h.Set(object.Symbol("open_timeout"), object.IntValue(7)) + h.Set(object.Symbol("read_timeout"), object.Float(0.5)) + f := &ftpObj{} + port, _, _, _, _, _ := vm.ftpApplyOpts(f, h, 21) + if port != 21 { + t.Errorf("port = %d, want 21 left alone when the Hash carries none", port) + } + if got := ftpTimeoutSeconds(f.openTimeout); got != 7*stdtime.Second { + t.Errorf("open_timeout = %v, want 7s -- this door is not wired", got) + } + if got := ftpTimeoutSeconds(f.readTimeout); got != 500*stdtime.Millisecond { + t.Errorf("read_timeout = %v, want 500ms", got) + } + }) + + // ⛔ nil is a REQUEST, not an absence. `read_timeout: nil` asks for NO bound + // and must not fall back to MRI's 60 s default, which is the whole reason + // ftpApplyOpts tests the key's PRESENCE rather than its value. A key absent + // from the Hash is the other case, and must leave the field untouched. + t.Run("an explicit nil is carried, an absent key is not", func(t *testing.T) { + h := object.NewHash() + h.Set(object.Symbol("read_timeout"), object.NilV) + f := &ftpObj{readTimeout: object.IntValue(60)} + vm.ftpApplyOpts(f, h, 21) + if !object.IsNil(f.readTimeout) { + t.Errorf("read_timeout = %v, want nil: an explicit nil asks for no bound", f.readTimeout) + } + if f.openTimeout != nil { + t.Errorf("open_timeout = %v, want untouched: it was absent from the Hash", f.openTimeout) + } + }) +} + +// ftpTimeoutErr's two raises, which the non-timeout test above cannot reach +// because it returns before them. MRI distinguishes Net::OpenTimeout from +// Net::ReadTimeout and ruby/spec asserts the CLASS, so a version that collapsed +// them would still satisfy any check that only asked "did it raise?". +func TestFTPTimeoutErrNamesThePhase(t *testing.T) { + for _, tc := range []struct{ phase, want string }{ + {"open", "Net::OpenTimeout"}, + {"read", "Net::ReadTimeout"}, + // There is no third class: anything that is not the open phase is a read. + {"", "Net::ReadTimeout"}, + } { + t.Run(tc.phase+"=>"+tc.want, func(t *testing.T) { + // WRAPPED, because that is what the callers hand it: the standard + // library returns a *net.OpError around the timeout, and only + // errors.As sees through it. A type assertion would miss this. + err := fmt.Errorf("dial tcp 127.0.0.1:21: %w", ftpStubTimeout{}) + + got, raised := func() (re RubyError, ok bool) { + defer func() { + if r := recover(); r != nil { + re, ok = r.(RubyError) + } + }() + ftpTimeoutErr(err, tc.phase) + return RubyError{}, false + }() + + if !raised { + t.Fatalf("ftpTimeoutErr returned for a timeout in phase %q; it must raise", tc.phase) + } + if got.Class != tc.want { + t.Errorf("raised %s, want %s", got.Class, tc.want) + } + // MRI's message for both, which ruby/spec's Timeout specs match on. + if got.Message != "execution expired" { + t.Errorf("message = %q, want %q", got.Message, "execution expired") + } + }) + } +} + +// ftpStubTimeout is a net.Error that reports a timeout. A real socket can +// produce one only by waiting out a deadline, which is a second of test time per +// case; the classification under test is the same either way. +type ftpStubTimeout struct{} + +func (ftpStubTimeout) Error() string { return "i/o timeout" } +func (ftpStubTimeout) Timeout() bool { return true } +func (ftpStubTimeout) Temporary() bool { return true } From b761ca09ea496047dc95902fc3aee38607bcc172 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Fri, 13 Nov 2026 08:19:14 +0100 Subject: [PATCH 7/7] The gate had the defect it was written to catch, and a waiver I wrote was false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both about this gate rather than about a binding, and both found by pulling on the one thread the gate could not see. 1. ⛔ THE GATE WAS BLIND TO conn.Read -- the commonest blocking socket call of all. It caught retrlines at net_ftp_bind.go:485 because that reads through bufio's ReadString, and MISSED retrbinary nineteen lines below it, reading the same data connection through conn.Read. Same connection, same defect, invisible because only one of the two spellings was in the list. The gate was wired at some of its doors -- which is the exact defect shape it exists to refuse. Read is now checked, but only in files that import "net": a Read on a bytes.Reader, a file or a protocol decoder is not a socket call, and a gate that cried wolf on those would be switched off within a week. On this tree the rule is exact for Read -- the six non-socket Read sites (crypto/rand, mail, images, openbao, grpc, sftp) are all in files importing no net, and the restriction leaves exactly two, both real. The gate now checks 33 calls where it checked 27. ⛔ WRITE IS DELIBERATELY ABSENT, and the measurement is the reason. I added it first and claimed in a comment that the file rule was exact for it too. It is NOT: Write reported eleven sites, EIGHT of them not network writes at all -- seven *bufio.Writer.Write, which only buffers (the socket write happens at Flush), and one bytes.Buffer in a file that does import net. Separating those needs the receiver's TYPE, not its file. Three real unwrapped conn writes do exist; they are written down as a COMMENT at the foot of the backlog, not as entries, because an entry that never reports fails this gate by design. The gap is on the page instead of in somebody's memory. 2. ⛔ A WAIVER I WROTE IN THIS BRANCH WAS FALSE. nethttpRawDial carried //gvl:released-by-caller every call site is inside the ioBlock at nethttpDial, so the GVL is already dropped when this runs There is no function called nethttpDial. The only ioBlock in nethttp_bind.go is in nethttpExchangeFramed, and the dial path does not go through it: nethttpRawExchange called nethttpDialXfer directly. So the whole dial -- the TCP connect, a proxy CONNECT (write, then read the reply a byte at a time) and the TLS handshake -- held the emulated GVL, stopping every other Ruby thread for the duration of a connect to an unreachable host. The slowest part of a request was the unwrapped part. A bare waiver is a silent one; a waiver with a reason that cannot be followed to a real release point is WORSE, because it reads as verified and nobody re-checks it. Fixed where it belongs: nethttpRawExchange now runs nethttpDialXfer inside an ioBlock, which covers dial, CONNECT and handshake at once. The two other waivers in that file name nethttpExchangeFramed and are correct. nethttpProxyConnect gets a waiver of its own, true as of this commit, and the negative control confirms the gate names nethttp_bind.go:646 the moment it is removed. Measured: gate green at 33 calls over 542 files; 127 tests pass, including the whole Net::HTTP suite with the dial now wrapped. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/gvl_blocking_io_test.go | 45 ++++++++++++++++++- internal/vm/nethttp_bind.go | 20 ++++++++- internal/vm/testdata/gvl-blocking-backlog.txt | 26 ++++++++++- 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/internal/vm/gvl_blocking_io_test.go b/internal/vm/gvl_blocking_io_test.go index fcb4b133..0c9a2d78 100644 --- a/internal/vm/gvl_blocking_io_test.go +++ b/internal/vm/gvl_blocking_io_test.go @@ -48,6 +48,32 @@ func TestEveryBlockingSocketCallReleasesTheGVL(t *testing.T) { "Accept": true, "Dial": true, "DialTimeout": true, "ReadString": true, "ReadFrom": true, "ReadFromUDP": true, "ReadFull": true, } + // ⛔ Read is the commonest blocking socket call of all, and the first version + // of this gate did not look for it -- which made it blind to retrbinary, + // nineteen lines below the retrlines it DID catch, reading the same data + // connection through conn.Read instead of bufio's ReadString. The gate had + // the very defect it was written to catch: wired at some of its doors. + // + // It is not in the list above because `Read` on a bytes.Reader, a file or a + // protocol decoder is not a socket call, and a gate that cried wolf on those + // would be switched off within a week. The discriminator is the FILE: one + // that does not import "net" cannot hold a net.Conn. Measured on this tree + // that is enough for Read -- the six non-socket Read sites (crypto/rand, + // mail, images, openbao, grpc, sftp) are all in files importing no net, and + // restricting to net-importing files leaves exactly two, both real. + // + // ⛔ WRITE IS DELIBERATELY ABSENT, and the measurement is why. Adding it + // reported eleven sites, of which eight are NOT network writes: seven + // `s.writer().Write(...)` on a *bufio.Writer, which only buffers (the socket + // write happens at Flush), and one `buf.Write(chunk)` on a bytes.Buffer in a + // file that does import net. So the file-level rule does NOT hold for Write; + // telling those apart needs the receiver's TYPE, not its file. Three real + // unwrapped conn writes do exist (socket_raw.go:206, net_ftp_bind.go:431, + // nethttp_bind.go's CONNECT). They are written down as a COMMENT at the foot + // of the backlog -- not as entries, since an entry that never reports fails + // this gate by design -- so the known gap is on the page instead of in + // somebody's memory. + connOnly := map[string]bool{"Read": true} // The wrappers that release it. A call inside one of these closures is fine; // ftpIO forwards to ioBlock, and a binding may add its own as long as it is // named here, which keeps this list the single place the exemption lives. @@ -78,6 +104,7 @@ func TestEveryBlockingSocketCallReleasesTheGVL(t *testing.T) { } scanned++ exempt := exemptLines(fset, f) + netFile := importsNet(f) // Walk with a stack so a call can be asked which closures enclose it. var stack []ast.Node @@ -93,7 +120,10 @@ func TestEveryBlockingSocketCallReleasesTheGVL(t *testing.T) { return true } sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || !blocking[sel.Sel.Name] { + if !ok { + return true + } + if !blocking[sel.Sel.Name] && !(netFile && connOnly[sel.Sel.Name]) { return true } calls++ @@ -182,6 +212,19 @@ func readBacklog(t *testing.T) (map[string]bool, []string) { return out, order } +// importsNet reports whether the file imports the standard "net" package, which +// is the cheap, exact discriminator for "could a `Read` here be a socket read?". +// It is deliberately the plain import path and not a prefix match: net/http and +// net/url carry no net.Conn of their own. +func importsNet(f *ast.File) bool { + for _, imp := range f.Imports { + if imp.Path != nil && imp.Path.Value == `"net"` { + return true + } + } + return false +} + // enclosedByReleaser reports whether any FuncLit on the stack is an argument to // one of the releasing wrappers. Lexical enclosure is the right test: ioBlock // takes the closure and runs it with the GVL dropped, so a call anywhere inside diff --git a/internal/vm/nethttp_bind.go b/internal/vm/nethttp_bind.go index 1d891b6e..45f936c8 100644 --- a/internal/vm/nethttp_bind.go +++ b/internal/vm/nethttp_bind.go @@ -476,7 +476,16 @@ func (vm *VM) nethttpRawExchange(cfg *nethttpXfer, method, path string, body []b } } if !reused { - stream, derr := vm.nethttpDialXfer(cfg) + // ⛔ THE WHOLE DIAL RELEASES THE GVL, not just the exchange below it. + // nethttpDialXfer blocks on the network three times over -- the TCP dial, + // a proxy CONNECT (write, then read the reply a byte at a time), and the + // TLS handshake -- and every one of them was holding the emulated GVL, + // which stops every other Ruby thread for the duration of a connect to an + // unreachable host. Only the exchange at nethttpExchangeFramed was + // wrapped, so the slowest part of a request was the unwrapped part. + var stream streamIO + var derr error + ioBlock(vm, func() { stream, derr = vm.nethttpDialXfer(cfg) }) if derr != nil { vm.raiseTransportErr(derr, "open") } @@ -617,6 +626,8 @@ func (vm *VM) nethttpDialXfer(cfg *nethttpXfer) (streamIO, error) { // the proxy's response, erroring unless it is a 2xx. It reads only up to the // response's header terminator (byte at a time) so no tunnelled TLS bytes are // consumed. +// +//gvl:released-by-caller its only non-test caller is nethttpDialXfer, which nethttpRawExchange runs inside an ioBlock func nethttpProxyConnect(conn net.Conn, target, proxyAuth string) error { var b strings.Builder b.WriteString("CONNECT " + target + " HTTP/1.1\r\n") @@ -758,7 +769,12 @@ func basicAuthHeader(user, pass string) string { // nethttpRawDial opens a raw TCP connection with an optional dial timeout // (open_timeout); 0 means no bound. // -//gvl:released-by-caller every call site is inside the ioBlock at nethttpDial, so the GVL is already dropped when this runs +// ⛔ The reason below used to name "the ioBlock at nethttpDial". There is no +// such function, and there was no ioBlock on this path at all: the waiver read +// as verified and was not. A waiver whose reason cannot be followed to a real +// release point is worse than a bare one, because nobody re-checks it. +// +//gvl:released-by-caller reached only through nethttpDialXfer, which nethttpRawExchange now calls inside an ioBlock func nethttpRawDial(host, port string, openTO time.Duration) (net.Conn, error) { addr := net.JoinHostPort(host, port) if openTO > 0 { diff --git a/internal/vm/testdata/gvl-blocking-backlog.txt b/internal/vm/testdata/gvl-blocking-backlog.txt index 9a0554ac..fb21fef0 100644 --- a/internal/vm/testdata/gvl-blocking-backlog.txt +++ b/internal/vm/testdata/gvl-blocking-backlog.txt @@ -18,10 +18,17 @@ # function //gvl:released-by-caller instead and delete the line. # Net::FTP's DATA connection. The control connection was fixed in #921/#916; -# these three were missed by that change and found by this gate. +# these were missed by that change and found by this gate. +# +# ⛔ :504 is retrbinary, NINETEEN LINES below the retrlines at :485. Same data +# connection, same defect — invisible to the first version of this gate because +# it reads through conn.Read and its neighbour reads through bufio's +# ReadString, and only the second name was in the list. The gate had the defect +# it exists to catch. net_ftp_bind.go:409 net_ftp_bind.go:419 net_ftp_bind.go:485 +net_ftp_bind.go:504 # Net::IMAP: readLine/readLiteral are handed to imap.NewReader as callbacks and # the binding has no ioBlock at all — the same shape Net::FTP had. @@ -35,3 +42,20 @@ net_smtp_bind.go:140 # beside its own client is the exact deadlock above. httprb.go:148 hanami_bind.go:514 + +# ⛔ KNOWN GAP — three unwrapped conn WRITES this gate cannot see yet. They are +# COMMENTS, not entries: an entry that never reports is a failure by design, and +# these are invisible to the current rule. Written here so the gap is on the +# page rather than in somebody's memory. +# +# socket_raw.go:206 s.conn.Write(msg) +# net_ftp_bind.go:431 conn.Write(data[off:end]) (the data connection) +# nethttp_bind.go conn.Write in nethttpProxyConnect (now covered by +# its caller's ioBlock, so this one is already fine) +# +# Why not gate on Write: adding it reported ELEVEN sites, eight of them not +# network writes at all — seven *bufio.Writer.Write (buffered; the socket write +# happens at Flush) and one bytes.Buffer in a file that does import net. The +# file-level rule that makes Read exact does NOT hold for Write; separating +# them needs the receiver's TYPE. Eight false alarms is how a gate gets +# switched off, so the rule stays narrow and the gap stays written down.