Net::FTP: the data connection releases the GVL too, and the backlog shrinks

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])

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 <reason> 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