From deaafc885f304b22e0e4ac3bd35a99ecb5ca47d0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Fri, 13 Nov 2026 13:42:56 +0100 Subject: [PATCH 1/2] Net::FTP: the DATA connection releases the GVL too, and the backlog shrinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #921 bounded the control connection and #924 released the GVL around it. Both left the DATA connection holding it, and the gate #924 added said so in its seeded backlog -- four entries I wrote down rather than fixed, so the gate could land without a sprawling change. This is the change. dialData, acceptData, sendData, retrlines and retrbinary now run their blocking call inside ftpIO, and the four entries are DELETED from testdata/gvl-blocking-backlog.txt. That list may only shrink, and an entry that stops reporting fails the gate too, so a fix cannot leave its allowance behind. ⛔ acceptData is the one #916 is actually about. In ACTIVE mode the client listens and the server dials back, so a Ruby program that serves itself has both ends in one process -- and holding the GVL in that Accept parks every other Ruby thread, INCLUDING the one that would make the call it is waiting for. ruby/spec's net-ftp fixtures are exactly that shape, and their spec_helper.rb puts Net::FTP into active mode (see #916's comment: rbgo has no Net::FTP.default_passive, so today that helper is skipped in silence and the suite never reaches this path at all). Only the blocking call goes inside each closure. ftpConnErr raises, and a panic crossing a released GVL would unwind without re-acquiring it, so every raise stays outside. ⛔ NO BEHAVIOURAL TEST, AND THE REASON IS NOT LAZINESS. The in-repo FTP test server is Go-side, so it cannot reproduce this deadlock at all: nothing about rbgo's emulated GVL blocks a Go goroutine, and a test built on it would pass whether or not the fix is present. A real regression test needs BOTH ends in Ruby -- a dummy FTP server in a Ruby Thread, which is what ruby/spec's fixture is -- and that is worth doing properly rather than approximating with something that looks like proof and is not. What does guard the property is the gate: TestEveryBlockingSocketCallReleasesTheGVL fails in about a second if any of these five is unwrapped again, and the backlog deletion means it also fails if one silently stops being reported. The measurement that would show this moving -- the library/net-ftp sweep -- is deferred: this machine has been carrying a 1-minute load average between 23 and 86, and a per-file timeout measured under that cannot tell a hang from a slow file. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp_bind.go | 30 +++++++++++++++---- internal/vm/testdata/gvl-blocking-backlog.txt | 26 +++++++--------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/internal/vm/net_ftp_bind.go b/internal/vm/net_ftp_bind.go index 00b6a6a5..f96b87d2 100644 --- a/internal/vm/net_ftp_bind.go +++ b/internal/vm/net_ftp_bind.go @@ -406,7 +406,11 @@ func (f *ftpObj) transfercmd(cmd string) net.Conn { // dialData dials a passive data address, raising Net::FTPConnectionError when the // connection cannot be made. func (f *ftpObj) dialData(host string, port int) net.Conn { - conn, err := net.Dial("tcp", net.JoinHostPort(host, strconv.Itoa(port))) + var conn net.Conn + var err error + // Only the dial goes inside: ftpConnErr raises, and a panic crossing a + // released GVL would unwind without re-acquiring it. + f.ftpIO(func() { conn, err = net.Dial("tcp", net.JoinHostPort(host, strconv.Itoa(port))) }) if err != nil { ftpConnErr(err) } @@ -415,8 +419,16 @@ func (f *ftpObj) dialData(host string, port int) net.Conn { // acceptData accepts the server's active-mode data callback, raising // Net::FTPConnectionError when the accept fails. +// ⛔ THIS IS THE ONE #916 IS ABOUT. In ACTIVE mode the client listens and the +// server dials back, so a Ruby program that serves itself has both ends in one +// process. Holding the GVL here parks every other Ruby thread -- including the +// one that would make the call this Accept is waiting for. ruby/spec's net-ftp +// fixtures run exactly that shape, and their spec_helper puts Net::FTP into +// active mode. func (f *ftpObj) acceptData(ln net.Listener) net.Conn { - conn, err := ln.Accept() + var conn net.Conn + var err error + f.ftpIO(func() { conn, err = ln.Accept() }) if err != nil { ftpConnErr(err) } @@ -428,7 +440,11 @@ func (f *ftpObj) acceptData(ln net.Listener) net.Conn { func (f *ftpObj) sendData(conn net.Conn, data []byte, blocksize int) { for off := 0; off < len(data); off += blocksize { end := min(off+blocksize, len(data)) - if _, err := conn.Write(data[off:end]); err != nil { + // A write blocks once the peer stops reading and the window fills, so it + // releases the GVL like every other call on this connection. + var err error + f.ftpIO(func() { _, err = conn.Write(data[off:end]) }) + if err != nil { conn.Close() ftpConnErr(err) } @@ -482,7 +498,9 @@ func (f *ftpObj) retrlines(cmd string, fn func(string)) { conn := f.transfercmd(cmd) br := bufio.NewReader(conn) for { - line, err := br.ReadString('\n') + var line string + var err error + f.ftpIO(func() { line, err = br.ReadString('\n') }) if line != "" { fn(ftpChomp(line)) } @@ -501,7 +519,9 @@ func (f *ftpObj) retrbinary(cmd string, blocksize int, fn func([]byte)) { conn := f.transfercmd(cmd) buf := make([]byte, blocksize) for { - n, err := conn.Read(buf) + var n int + var err error + f.ftpIO(func() { n, err = conn.Read(buf) }) if n > 0 { chunk := make([]byte, n) copy(chunk, buf[:n]) diff --git a/internal/vm/testdata/gvl-blocking-backlog.txt b/internal/vm/testdata/gvl-blocking-backlog.txt index fb21fef0..9bac363d 100644 --- a/internal/vm/testdata/gvl-blocking-backlog.txt +++ b/internal/vm/testdata/gvl-blocking-backlog.txt @@ -17,18 +17,14 @@ # 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 were missed by that change and found by this gate. +# Net::FTP's DATA connection — FIXED and removed from this list, which is the +# only direction it may move. dialData, acceptData, sendData, retrlines and +# retrbinary all run their blocking call inside ftpIO now. # -# ⛔ :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 +# acceptData was the one #916 is about: in ACTIVE mode the client listens and +# the server dials back, so a program that serves itself has both ends in one +# process and the Accept was parking every other Ruby thread — including the one +# that would have made the call it was waiting for. # 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. @@ -48,10 +44,10 @@ hanami_bind.go:514 # 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) +# socket_raw.go:206 s.conn.Write(msg) (still unwrapped) +# net_ftp_bind.go conn.Write in sendData (FIXED: ftpIO) +# nethttp_bind.go conn.Write in nethttpProxyConnect (covered by its +# caller's ioBlock) # # 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 From f90469af44f2e51308ea700c3ebd78d5e1096017 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Fri, 13 Nov 2026 13:57:28 +0100 Subject: [PATCH 2/2] Net::FTP.default_passive, and the options a nil host was throwing away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ruby/spec's library/net-ftp/spec_helper.rb — which all 56 files require — opens with: if defined?(Net::FTP.default_passive) Net::FTP.default_passive = false end rbgo had no such accessor, so the guard was skipped in SILENCE and the whole suite ran against a connection rbgo kept in PASSIVE mode, while the fixture server implements PORT/EPRT and has no `pasv` method at all. That is the `451 Exception: undefined method 'pasv'` those specs report (#916). Implemented against the reference, not against expectations: net-ftp 0.3.9 lib/net/ftp.rb:96 (`@@default_passive = true`), :159-167 (the accessors) and :286-290 (`if options[:passive].nil?` → the default). The value lives in the CLASS VARIABLE MRI uses, so Net::FTP.class_variable_get(:@@default_passive) answers here as it does there and there is one place holding it, not two that can disagree. Two defects the reference comparison turned up that reading our own code would not have: - ⛔ `Net::FTP.new(nil, passive: true)` answered FALSE. ftpInit returned before reading the options Hash whenever the host was nil, so EVERY option was dropped on that path — binary, timeouts, credentials, all of it. MRI applies the options first and connects separately, and ruby/spec drives exactly that shape: `@ftp.send(:initialize, nil, options)` then asserts `passive` (initialize_spec.rb:201-226). A mechanism wired at only some of its doors, again. - ⛔ `passive: nil` meant FALSE. ftpApplyOpts tested the KEY's presence, but MRI tests the VALUE: nil is "no opinion" and falls back to the default. Note this is the OPPOSITE of read_timeout in the same Hash three lines away, where nil IS a request (for no bound) — easy to conflate, so both now say which they are in a comment. Tests: a 12-line table run through rbgo, each line annotated with the reference line it comes from, plus ftpDefaultPassive's fallbacks (nil class, absent or nil class variable → true), which no Ruby program can reach and which decide the transfer mode of every connection. ⛔ Measured before pushing with the tool added in #925, which caught my own gap: `covratchet -require ftpDefaultPassive,ftpApplyOpts,ftpInit` first reported ftpDefaultPassive at 66.7%, and reports all three at 100% now. Refs #916 Co-Authored-By: Claude Opus 5 --- internal/vm/net_ftp.go | 52 +++++++++++++++++-- internal/vm/net_ftp_bind.go | 23 +++++++-- internal/vm/net_ftp_internal_test.go | 77 ++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 8 deletions(-) diff --git a/internal/vm/net_ftp.go b/internal/vm/net_ftp.go index a217045c..d5ab9522 100644 --- a/internal/vm/net_ftp.go +++ b/internal/vm/net_ftp.go @@ -77,6 +77,29 @@ func (vm *VM) registerFTPErrors(netMod, std *RClass) { // options) connects (and logs in when a username is supplied); open is new plus // the ensured-close block form. func (vm *VM) registerFTPClassMethods(ftp *RClass) { + // Net::FTP.default_passive, held in the CLASS VARIABLE the reference + // implementation uses (net-ftp 0.3.9 lib/net/ftp.rb:96 `@@default_passive = + // true`, accessors at :159-167). Storing it there rather than in a Go field + // is not decoration: a Ruby program can reach it through + // Net::FTP.class_variable_get(:@@default_passive), as it can under MRI, and + // there is one place holding the value instead of two that can disagree. + // + // ⛔ ruby/spec's library/net-ftp/spec_helper.rb sets this to FALSE for the + // whole suite, and its fixture server implements PORT/EPRT and has no `pasv` + // method at all. Without the accessor the helper's `if + // defined?(Net::FTP.default_passive)` guard was skipped in SILENCE and every + // spec ran against a mode the fixture cannot speak (#916). + ftp.cvars["@@default_passive"] = object.Bool(true) + ftp.smethods["default_passive"] = &Method{name: "default_passive", owner: ftp, + native: func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { + return object.Bool(ftpDefaultPassive(ftp)) + }} + ftp.smethods["default_passive="] = &Method{name: "default_passive=", owner: ftp, + native: func(_ *VM, _ object.Value, a []object.Value, _ *Proc) object.Value { + ftp.cvars["@@default_passive"] = object.Bool(a[0].Truthy()) + return a[0] + }} + newFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { f := newFTPObj(vm, ftp) vm.ftpInit(f, args) @@ -109,20 +132,33 @@ func (vm *VM) ftpWithClose(f *ftpObj, blk *Proc) (res object.Value) { // the legacy positional user / passwd / acct, and a login when a username is // present. func (vm *VM) ftpInit(f *ftpObj, args []object.Value) { - if len(args) == 0 || object.IsNil(args[0]) { - return - } - host := strArg(args[0]) port := 21 user, passwd, acct := "", "", "" userGiven, passwdGiven := false, false + // ⛔ THE OPTIONS ARE APPLIED WHETHER OR NOT A HOST WAS GIVEN. This used to + // return before reading them when args[0] was nil, so + // `Net::FTP.new(nil, passive: true)` silently ignored EVERY option -- found + // by running the settings against the reference rather than against itself: + // rbgo answered `false` where net-ftp 0.3.9 answers `true`. + // + // MRI's initialize sets @binary / @passive / @debug_mode / the timeouts + // first and only then, separately, connects when a host is present + // (lib/net/ftp.rb:285-300). ruby/spec drives exactly that shape: + // `@ftp.send(:initialize, nil, options)` then asserts `passive` + // (library/net-ftp/initialize_spec.rb:201-226). if len(args) >= 2 { if h, ok := args[1].(*object.Hash); ok { port, user, userGiven, passwd, passwdGiven, acct = vm.ftpApplyOpts(f, h, port) } } + // No host: configured, not connected. + if len(args) == 0 || object.IsNil(args[0]) { + return + } + host := strArg(args[0]) + vm.ftpConnect(f, host, port) if userGiven { vm.ftpLogin(f, user, passwd, passwdGiven, acct) @@ -139,7 +175,13 @@ func (vm *VM) ftpApplyOpts(f *ftpObj, h *object.Hash, port int) (int, string, bo port = clampInt(n) } } - if v, ok := ftpOptHash(h, "passive"); ok { + // ⛔ MRI tests the VALUE, not the key: `if options[:passive].nil?` falls back + // to @@default_passive (ftp.rb:286-290). So `passive: nil` is NOT a request + // for active mode — it is "no opinion". Reading the key's presence instead + // would make an explicit nil mean false, which is the opposite. This differs + // from read_timeout in the same Hash, where nil IS a request (for no bound), + // and the two are easy to conflate because they sit three lines apart. + if v, ok := ftpOptHash(h, "passive"); ok && !object.IsNil(v) { f.passive = v.Truthy() } // open_timeout and read_timeout are set from the options Hash exactly as diff --git a/internal/vm/net_ftp_bind.go b/internal/vm/net_ftp_bind.go index f96b87d2..0817d95b 100644 --- a/internal/vm/net_ftp_bind.go +++ b/internal/vm/net_ftp_bind.go @@ -98,11 +98,28 @@ 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. +// ftpDefaultPassive reads Net::FTP's @@default_passive. An absent or non-boolean +// value reads as MRI's documented default, true: this is consulted on every +// construction, and a class variable someone deleted must not silently flip the +// transfer mode of the next connection. +func ftpDefaultPassive(cls *RClass) bool { + if cls == nil { + return true + } + v, ok := cls.cvars["@@default_passive"] + if !ok || object.IsNil(v) { + return true + } + return v.Truthy() +} + func newFTPObj(vm *VM, cls *RClass) *ftpObj { return &ftpObj{ - cls: cls, - binary: true, - passive: true, + cls: cls, + binary: true, + // Read at CONSTRUCTION, as MRI does: Net::FTP.default_passive = false + // affects connections made after it, never one already open. + passive: ftpDefaultPassive(cls), openTimeout: object.NilV, readTimeout: object.IntValue(ftpDefaultReadTimeout), vm: vm, diff --git a/internal/vm/net_ftp_internal_test.go b/internal/vm/net_ftp_internal_test.go index 98b733fe..71cf7000 100644 --- a/internal/vm/net_ftp_internal_test.go +++ b/internal/vm/net_ftp_internal_test.go @@ -1696,6 +1696,83 @@ func TestFTPTimeoutErrIgnoresANonTimeout(t *testing.T) { ftpTimeoutErr(nil, "open") } +// Net::FTP.default_passive, and the two things that made it worth running +// against the reference rather than against our own expectations. +// +// ⛔ WHY IT MATTERS BEYOND THE ACCESSOR: ruby/spec's library/net-ftp/spec_helper.rb +// is `if defined?(Net::FTP.default_passive) then Net::FTP.default_passive = false`, +// and its fixture server implements PORT/EPRT with NO `pasv` method at all. +// While the accessor was missing, that guard was skipped in SILENCE and all 56 +// files ran in a mode the fixture cannot speak (#916). +// +// ⛔ AND THE DEFECT THE COMPARISON FOUND: `Net::FTP.new(nil, passive: true)` +// answered false. ftpInit returned before reading the options Hash whenever the +// host was nil, so EVERY option was dropped on that path -- the shape of a +// mechanism wired at only some of its doors, again. MRI applies the options +// first and connects separately (net-ftp 0.3.9 lib/net/ftp.rb:285-300), which is +// exactly what library/net-ftp/initialize_spec.rb:201-226 drives with +// `@ftp.send(:initialize, nil, options)`. +func TestFTPDefaultPassiveMatchesTheReference(t *testing.T) { + got := runSrc(t, `require "net/ftp" +p Net::FTP.default_passive +p Net::FTP.new.passive +before = Net::FTP.new +Net::FTP.default_passive = false +p Net::FTP.default_passive +p before.passive +p Net::FTP.new.passive +p Net::FTP.new(nil, passive: nil).passive +p Net::FTP.new(nil, passive: true).passive +p Net::FTP.new(nil, passive: false).passive +Net::FTP.default_passive = true +p Net::FTP.new(nil, passive: nil).passive +p Net::FTP.new(nil, passive: false).passive +p(Net::FTP.default_passive = false) +p Net::FTP.class_variable_get(:@@default_passive) +`) + // Each line is net-ftp 0.3.9's answer, read off lib/net/ftp.rb:96 (the + // default), :159-167 (the accessors) and :286-290 (nil means "no opinion", + // so it falls back to the default while false and true are requests). + want := "true\n" + // default_passive starts true + "true\n" + // so does a fresh connection + "false\n" + // the setter takes + "true\n" + // ⛔ a connection built BEFORE the change keeps its mode + "false\n" + // one built after follows it + "false\n" + // passive: nil is no opinion -> the default + "true\n" + // passive: true is a request, even with a nil host + "false\n" + // and so is passive: false + "true\n" + // default back to true: nil follows it + "false\n" + // while an explicit false still wins + "false\n" + // the writer returns its argument + "false" // and the value lives in the class variable MRI uses (runSrc trims the last newline) + if got != want { + t.Errorf("default_passive table:\ngot %q\nwant %q", got, want) + } +} + +// ftpDefaultPassive's fallbacks, which no Ruby program can reach: it is +// consulted on EVERY construction, so a missing or nil class variable must read +// as MRI's documented default rather than silently flipping the transfer mode of +// the next connection. Asserted directly because "it cannot happen" is how a +// default nobody chose gets shipped. +func TestFTPDefaultPassiveFallsBackToMRIsDefault(t *testing.T) { + if !ftpDefaultPassive(nil) { + t.Error("a nil class must read as true, MRI's default") + } + cls := &RClass{cvars: map[string]object.Value{}} + if !ftpDefaultPassive(cls) { + t.Error("an absent @@default_passive must read as true") + } + cls.cvars["@@default_passive"] = object.NilV + if !ftpDefaultPassive(cls) { + t.Error("a nil @@default_passive must read as true, not as false") + } + cls.cvars["@@default_passive"] = object.Bool(false) + if ftpDefaultPassive(cls) { + t.Error("an explicit false must be honoured") + } +} + // ⛔ 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