Skip to content

Net::FTP releases the GVL, and a gate so the next binding cannot forget - #924

Merged
tannevaled merged 9 commits into
mainfrom
ftp-release-gvl
Nov 13, 2026
Merged

tannevaled merged 9 commits into
mainfrom
ftp-release-gvl

Conversation

@tannevaled

Copy link
Copy Markdown
Contributor

Net::FTP releases the GVL, and a gate so the next binding cannot forget

Stacked on #921 (ftp-bound-reads); the diff to review is the last three
commits.

What this changes

Net::FTP's control connection now runs its blocking socket calls inside
ioBlock, the same wrapper Net::HTTP has used at twenty sites since it was
written. rbgo emulates a GVL, so a native call that blocks while holding it
stops every other Ruby thread.

The cost was never 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.

Proved by dose-response rather than by correlation, because "stuck or merely
late?" is a question only a clock answers:

client deadline accept completed at
2 s 2.308 s
5 s 5.307 s
with ioBlock 0.305 s

The accept was not slow. It was waiting for the client to give up.

⛔ Two limits I am not going to bury

1. This does not fix #916. The library/net-ftp specs are byte-identical
after the change: still 39 killed by the clock, still 459/17, still −442. The
GVL release is proved on a minimal reproduction and it is necessary, but at
least one more cause remains and it is in the spec-harness path. I am saying so
here rather than letting a green PR imply the issue is closed.

2. My own FTP fix was incomplete, and the gate caught me, not the other way
round.
I wrapped the control connection and left the data connection
(Dial / Accept / ReadString) holding the GVL. The gate saw it; I did not.
Those three sites are in the backlog below, unfixed and named.

The gate

TestEveryBlockingSocketCallReleasesTheGVL walks the AST of every
non-test file in internal/vm (542 files, 27 blocking calls) and asks the
question that actually matters. "Is ioBlock implemented?" was always the wrong
one — it was implemented, and correct, for years. The right question is "who
else should be calling it?"
, and that is answerable statically: a blocking
socket call in a binding must sit lexically inside a closure handed to
ioBlock / threadBlock / threadRelease.

This is the defect shape this repository has now produced eight times in one
week
: a mechanism that exists and is wired at only some of its doors
(Array.new vs Array#first; expand_path vs Dir.home; raiseWithIvars vs
Kernel#raise; paren-FCALL vs the vcall flag; Set's constructors; the stub
list across three surfaces; the README's derived numbers; and now ioBlock in
Net::HTTP but not Net::FTP). A lesson that recurs is a missing gate, not a
missing note, so it is a gate.

It lands as a ratchet, not a big bang

It was written after the fact and finds sites in five 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.
testdata/gvl-blocking-backlog.txt records what was already there on the day it
was written, so the gate fires on what happens next. The list may only
shrink: an entry that stops reporting also fails, because a stale allowance
is exactly where the next regression hides.

What it found beyond FTP:

site what it is
net_ftp_bind.go ×3 the data connection my own fix missed
net_imap_bind.go ×2 readLine/readLiteral passed as callbacks, no ioBlock at all — FTP's shape exactly
net_smtp_bind.go the response reader
httprb.go, hanami_bind.go two Accepts: an embedder running one beside its own client is the deadlock above

The exemption is named, never inferred

A purely lexical rule cannot see a helper whose callers wrap it. I claimed
mid-review that Net::HTTP had four unwrapped sites; that was wrong — all
six findings there are helpers reached only from inside an ioBlock. So the
waiver is explicit, //gvl:released-by-caller <reason>, and the reason is
required
: a bare directive is a silent waiver, and silent waivers are how
Net::FTP went twenty sites without the wrapper everything else had. It also
keeps every exemption readable in one grep instead of hidden in an
interprocedural inference nobody can audit.

Controlled in both directions

Removing the single ftpIO wrapper makes the gate name net_ftp_bind.go:249
immediately. And it refuses to pass on silence: if it parses zero files or finds
zero blocking calls it fails, because an unread tree and a clean one look
identical from the outside.

Also here

The two functions #921's first coverage pass left uncovered — ftpApplyOpts's
open_timeout door and ftpTimeoutErr's two raises. That pass did not move the
measurement; this one was measured before pushing (go tool cover -func: both
100.0%).

Refs #916

🤖 Generated with Claude Code

tannevaled and others added 7 commits November 11, 2026 09:29
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The WIP commit below added open_timeout/read_timeout; this adds what it
owed.

A server that accepts and stays SILENT -- the production shape, and
exactly what #916's SIGQUIT dump showed rbgo waiting on -- must raise
Net::ReadTimeout. The connection is held OPEN on purpose: a server that
closed it would give the reader an EOF, a different path
(Net::FTPConnectionError), and the test would pass with no deadline at
all.

⛔ THE ELAPSED TIME IS PART OF THE ASSERTION. Without it the test would
also pass if the greeting read failed instantly for an unrelated reason,
which is the one outcome that would NOT prove a deadline fired. It
asserts the call took at least 150ms against a 0.2s read_timeout, and
measures 0.21s.

Control run in both directions: removing the single armRead() call makes
that test HANG until the 40s wall instead of failing in 0.21s.

The second test pins the defaults together -- open_timeout nil,
read_timeout 60 -- because the ASYMMETRY is the point and is MRI's, from
library/net-ftp/initialize_spec.rb:272 and :291. A tidier symmetric
default would change behaviour MRI specifies.

Refs #916

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measurement pending; see #916 for the mechanism.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#921's first push dropped internal/vm to 98.8% and the coverage ratchet
refused it. The uncovered lines were all branches no socket test can
reach, and each is a behaviour worth asserting on its own rather than a
coverage chore:

  - ftpTimeoutSeconds across every shape: nil (MRI's unbounded default),
    Integer, Float, and ⛔ the non-positive cases, which are UNBOUNDED
    and not "expire immediately" -- a deadline of now would fail every
    read at once -- plus a non-numeric, which is ignored rather than
    guessed at.
  - armRead's two early paths: an ftpObj with no connection yet, and a
    nil read_timeout that must CLEAR a deadline an earlier setting left
    behind. The second is asserted by showing the read goes back to
    BLOCKING, since a cleared deadline has no other visible effect.
  - ftpTimeoutErr returning for an error that is not a timeout. A version
    that raised unconditionally would turn every connection failure into
    Net::ReadTimeout, which is the opposite of what #916 needs to stay
    distinguishable.

Refs #916

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rbgo emulates a GVL: one Ruby thread runs VM code at a time, and a thread
that blocks in a native call MUST release it (ioBlock). Net::HTTP has done
that since it was written. Net::FTP did it at NONE of its sites, so every
read held the GVL across a blocking read(2).

The cost was not slowness, 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 the lock the client sits on.
Dose-response: a 2s client deadline put the accept at 2.308s, 5s at
5.307s, ioBlock at 0.305s. In ruby/spec: 39 of 56 library/net-ftp files
killed by the clock, none under MRI (#916).

⛔ THE POINT IS NOT THIS BUG, IT IS ITS SHAPE. Eight times in one week a
mechanism existed and was wired at only some of its doors: Array.new vs
Array#first, expand_path vs Dir.home, raiseWithIvars vs Kernel#raise, the
vcall flag, Set's constructors, the stubbed-helper list in three
surfaces, the README's derived numbers, and now ioBlock. "Is it
implemented?" was always the wrong question; "who else should be calling
it?" is answerable from the AST without running anything.

What it found beyond net-ftp, on its first run:

  - THREE more sites in Net::FTP itself -- the DATA connection, which my
    own fix in #921 missed;
  - Net::IMAP, whose readLine/readLiteral are handed to the library as
    callbacks with no ioBlock anywhere: the same shape, untouched;
  - Net::SMTP's response reader; httprb and hanami's Accept.

⛔ And it corrected me twice while being written. I had said "Net::HTTP
has four unwrapped sites": false. All six nethttp findings are helpers
whose callers already wrap them, which a purely lexical rule cannot see.
That exemption is NAMED, never inferred -- //gvl:released-by-caller
<reason>, a reason required, so every waiver is one grep away. A gate
that cries wolf is switched off within a week.

It lands as a RATCHET, not a big bang: testdata/gvl-blocking-backlog.txt
records what was already there, so the gate fires on what happens NEXT.
The list may only shrink -- an entry that stops reporting fails too,
because a stale allowance is where the next regression hides.

Control run in both directions: removing the single ftpIO wrapper makes
it name net_ftp_bind.go:249 immediately.

Refs #916

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3cd3b06 was meant to answer the ratchet and did not move the measurement:
CI came back with the same two functions, ftpApplyOpts at 95.0% and
ftpTimeoutErr at 80.0%. A fix that does not move the number is not a fix,
so this time the number is measured before pushing -- both now read 100.0%
under `go tool cover -func` over the FTP tests.

What was actually uncovered is the defect shape this repository keeps
repeating: a mechanism wired at only SOME of its doors.

  - ftpApplyOpts reads open_timeout and read_timeout from the options Hash
    on two adjacent lines. Every socket test passes read_timeout ALONE, and
    the accessor tests go through the attr writer, not the Hash -- so the
    open_timeout line had never run. The new test goes through the Hash
    door for BOTH, and asserts the other half of that contract too: an
    explicit `read_timeout: nil` is a REQUEST for no bound and must not
    fall back to MRI's 60 s default, while a key absent from the Hash must
    leave the field untouched. Those are the two cases the key-presence
    check exists to tell apart.

  - ftpTimeoutErr has two raises and the existing test returns before
    reaching either: it only asserts that a NON-timeout is passed through.
    The new test drives a real net.Error timeout through both phases and
    asserts the CLASS -- Net::OpenTimeout for open, Net::ReadTimeout
    otherwise -- because MRI distinguishes them and ruby/spec matches on
    the class, so a version that collapsed the two would still satisfy any
    check that only asked "did it raise?". The error is WRAPPED, as the
    standard library hands it to the callers, so the test fails if the
    classification is ever narrowed to a type assertion.

Refs #916

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled
tannevaled changed the base branch from ftp-bound-reads to main November 13, 2026 07:03
tannevaled and others added 2 commits November 13, 2026 08:05
# Conflicts:
#	internal/vm/net_ftp.go
#	internal/vm/net_ftp_bind.go
… was false

Two findings, both about this gate rather than about a binding, and both
found by pulling on the one thread the gate could not see.

1. ⛔ THE GATE WAS BLIND TO conn.Read -- the commonest blocking socket call
   of all. It caught retrlines at net_ftp_bind.go:485 because that reads
   through bufio's ReadString, and MISSED retrbinary nineteen lines below
   it, reading the same data connection through conn.Read. Same connection,
   same defect, invisible because only one of the two spellings was in the
   list. The gate was wired at some of its doors -- which is the exact
   defect shape it exists to refuse.

   Read is now checked, but only in files that import "net": a 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. On
   this tree the rule is exact for Read -- the six non-socket Read sites
   (crypto/rand, mail, images, openbao, grpc, sftp) are all in files
   importing no net, and the restriction leaves exactly two, both real.
   The gate now checks 33 calls where it checked 27.

   ⛔ WRITE IS DELIBERATELY ABSENT, and the measurement is the reason. I
   added it first and claimed in a comment that the file rule was exact for
   it too. It is NOT: Write reported eleven sites, EIGHT of them not network
   writes at all -- seven *bufio.Writer.Write, which only buffers (the
   socket write happens at Flush), and one bytes.Buffer in a file that does
   import net. Separating those needs the receiver's TYPE, not its file.
   Three real unwrapped conn writes do exist; they are written down as a
   COMMENT at the foot of the backlog, not as entries, because an entry that
   never reports fails this gate by design. The gap is on the page instead
   of in somebody's memory.

2. ⛔ A WAIVER I WROTE IN THIS BRANCH WAS FALSE. nethttpRawDial carried

       //gvl:released-by-caller every call site is inside the ioBlock at
       nethttpDial, so the GVL is already dropped when this runs

   There is no function called nethttpDial. The only ioBlock in
   nethttp_bind.go is in nethttpExchangeFramed, and the dial path does not
   go through it: nethttpRawExchange called nethttpDialXfer directly. So the
   whole dial -- the TCP connect, a proxy CONNECT (write, then read the
   reply a byte at a time) and the TLS handshake -- held the emulated GVL,
   stopping every other Ruby thread for the duration of a connect to an
   unreachable host. The slowest part of a request was the unwrapped part.

   A bare waiver is a silent one; a waiver with a reason that cannot be
   followed to a real release point is WORSE, because it reads as verified
   and nobody re-checks it.

   Fixed where it belongs: nethttpRawExchange now runs nethttpDialXfer
   inside an ioBlock, which covers dial, CONNECT and handshake at once. The
   two other waivers in that file name nethttpExchangeFramed and are
   correct. nethttpProxyConnect gets a waiver of its own, true as of this
   commit, and the negative control confirms the gate names
   nethttp_bind.go:646 the moment it is removed.

Measured: gate green at 33 calls over 542 files; 127 tests pass, including
the whole Net::HTTP suite with the dial now wrapped.

Refs #916

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled
tannevaled merged commit 1b8e177 into main Nov 13, 2026
26 checks passed
@tannevaled
tannevaled deleted the ftp-release-gvl branch November 13, 2026 12:20
tannevaled added a commit that referenced this pull request Nov 13, 2026
…changed (#926)

The "a deadline belongs to the engine's API" section is the worked example
for the whole shared-engine argument, and it described the binding in the
past tense as having no timeout and no read deadline "anywhere". That was
true when it was written and is not true now: #921 is merged. A reader
arriving at that page would conclude rbgo still has neither.

The argument is unchanged -- an unbounded read in an engine four languages
share is still a shared denial of service, and the neutral engine should
still take a deadline in its own API. What changed is the consumer's half,
so that is what the new subsection records:

  - open_timeout (default nil, unbounded) and read_timeout (default 60),
    from the options Hash and from the attr writers, with the deadline armed
    PER READ. Non-positive means no bound, as in MRI.
  - the GVL released around the control connection, plus an AST gate that
    refuses a new blocking socket call holding it (#924), with the
    dose-response that proved the mechanism.

⛔ And the honest limit, which is the reason the extraction warning STAYS:
the measurement did not move. The library/net-ftp specs are byte-identical
after both changes -- still 39 killed, 459/17, -442. Both fixes are
necessary and proved on a minimal reproduction, but at least one cause
remains and it is in the spec-harness path. The three candidates eliminated
so far are named, with how they were eliminated (by reading, not by timing,
since the machine was too loaded for a timeout to mean anything).

Writing "fixed" here while the number sits where it was is exactly how a
docs page stops being evidence.

Refs #916

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tannevaled added a commit that referenced this pull request Nov 13, 2026
…hrinks (#927)

#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 <noreply@anthropic.com>
tannevaled added a commit that referenced this pull request Nov 13, 2026
#928)

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

#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 <noreply@anthropic.com>

* Net::FTP.default_passive, and the options a nil host was throwing away

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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Network specs hang: 75 ruby/spec files killed by the clock on rbgo and not on MRI (Net::FTP 39, Net::HTTP 27, Socket 8, Timeout 1)

1 participant