From deaafc885f304b22e0e4ac3bd35a99ecb5ca47d0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Fri, 13 Nov 2026 13:42:56 +0100 Subject: [PATCH] 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