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