Skip to content

Fail the write when the peer is gone and fwrite() reports nothing - #116

Open
wtsergo wants to merge 1 commit into
amphp:2.xfrom
wtsergo:fix/fail-write-when-peer-is-gone
Open

Fail the write when the peer is gone and fwrite() reports nothing#116
wtsergo wants to merge 1 commit into
amphp:2.xfrom
wtsergo:fix/fail-write-when-peer-is-gone

Conversation

@wtsergo

Copy link
Copy Markdown

Fixes#115.

WritableResourceStream's onWritable callback treats a write as failed only when fwrite() returned zero and a PHP error was captured:

if ($written === 0 && $errorCode !== 0 && $firstWrite) {

On a TLS stream whose peer is gone that is not sufficient on every OpenSSL build. fwrite() returns int(0) — never false — and once the stream's EOF flag has been set, some builds stop reporting the failure, leaving $errorCode at 0 while write(2) still returns EPIPE. The failed write is then mistaken for a partial write, the chunk is unshifted back onto the queue and the callback returns. Since the finally only disables or cancels the watcher when $writes is empty, the writable watcher is left enabled on a dead descriptor — permanently writable to the event loop — and the callback re-fires forever.

The fiber blocked in write() is never resumed, so nothing downstream ever gets the chance to close the socket; the descriptor leaks along with the spin.

What sets the EOF flag is ReadableResourceStream's own read callback ($data === '' && \feof($stream)) on the same underlying resource — so a server that simply reads its client to EOF is what silences the write side.

Changes

  • src/WritableResourceStream.php — also treat a no-progress write on a stream at EOF as a failure, and word the exception accordingly when no error was reported.
  • test/WritableResourceStreamTest.php — regression test: establish a TLS connection over loopback, close the peer, read to EOF, then write. Without the fix the write never returns and the test hits its 5s limit; with it, write() throws StreamException.

Because the underlying fwrite() behaviour is build-dependent, the test probes it first and skips where the platform still reports failed writes — on those builds the existing $errorCode check already catches it and there is nothing to exercise:

buildfwrite() after read-to-EOFtest
PHP 8.4.21 / OpenSSL 3.0.13ret=0, nothing reportedruns
PHP 8.4.24 / newer OpenSSLret=0, reports every timeskipped

On an affected build, without the fix:

1) Amp\ByteStream\WritableResourceStreamTest::testWriteFailsWhenTlsPeerIsGone
Expected test to complete before 5.000s time limit

and with it: OK (1 test, 3 assertions).

Notes on the condition

$firstWrite is deliberately kept, so the macOS/FreeBSD buffer-refill case the original guard was written for is unaffected — and on a genuinely dead socket no earlier chunk in the same invocation can have succeeded, so it is always true there.

The shape worth considering is a client that half-closes its write side while still reading, since \feof() is true there on a perfectly writable socket. That is safe, because the callback only runs when the loop reports writability, so a zero-byte write inside it already means something is wrong. I verified it by pushing 2 MiB to such a client through a 2 KiB receive window with a deliberately slow reader: every byte arrives, with and without this change.

Full suite passes (139 tests, 595 assertions). The one failure I see, testUploadBiggerBlockSecure, is unrelated — it talks to the live httpbin.org, which currently answers 503 Service Temporarily Unavailable.

@wtsergo
wtsergoforce-pushed the fix/fail-write-when-peer-is-gone branch from 6e226bc to 699c051CompareAugust 21, 2026 11:43
@wtsergo

Copy link
Copy Markdown
Author

CI is red here, but not on anything this PR touches — flagging it so it doesn't cost you a review cycle.

The only failing test in every run is the pre-existing testUploadBiggerBlockSecure, which talks to the live httpbin.org:

1) Amp\ByteStream\WritableResourceStreamTest::testUploadBiggerBlockSecure
Failed asserting that 'HTTP/1.1 503 Service Temporarily Unavailable
Server: awselb/2.0

It got a complete HTTP response back, so the 256 KiB TLS write itself succeeded — the assertion fails purely on the response body. Nothing in this change can produce that: the new branch only fires when a write makes no progress on a stream that is already at EOF.

httpbin.org appears to be answering 503 to GitHub's runners specifically. It fails there on two runs a day apart (2026-08-20 and 2026-08-21), while the same test passes from my machine against pristine 2.x (fd8db31, without this patch). I re-pushed the identical commit to retrigger CI in case it was transient; same result.

The other jobs are not independent failures — Windows and macOS never ran tests, their logs end at ##[error]The operation was canceled during Setup PHP, cancelled by fail-fast once a Linux job went red.

The new test does run and pass on your Ubuntu runners. In the progress line ...........F. covering tests 127–139, the F is test 138 (testUploadBiggerBlockSecure) and 139 — testWriteFailsWhenTlsPeerIsGone — is a passing .. Worth noting because that test skips itself on builds where fwrite() still reports failed writes to a closed TLS peer; on my machine it skips, on your runners it runs. So the regression coverage is live in your CI, not vacuous.

Happy to rebase, or to split out a fix that makes testUploadBiggerBlockSecure resilient, if either would help — just say which.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

WritableResourceStream spins at 100% CPU when a TLS peer is gone and fwrite() reports nothing

1 participant

@wtsergo