diff --git a/internal/vm/gvl_blocking_io_test.go b/internal/vm/gvl_blocking_io_test.go new file mode 100644 index 00000000..0c9a2d78 --- /dev/null +++ b/internal/vm/gvl_blocking_io_test.go @@ -0,0 +1,302 @@ +// 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, + } + // ⛔ 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. + 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) + netFile := importsNet(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 { + return true + } + if !blocking[sel.Sel.Name] && !(netFile && connOnly[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 +} + +// 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 +// 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/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) } } diff --git a/internal/vm/nethttp_bind.go b/internal/vm/nethttp_bind.go index c088f345..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") @@ -757,6 +768,13 @@ func basicAuthHeader(user, pass string) string { // nethttpRawDial opens a raw TCP connection with an optional dial timeout // (open_timeout); 0 means no bound. +// +// ⛔ 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 { @@ -807,6 +825,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 +886,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..fb21fef0 --- /dev/null +++ b/internal/vm/testdata/gvl-blocking-backlog.txt @@ -0,0 +1,61 @@ +# 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 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. +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 + +# ⛔ 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.