Repository navigation
Conversation
|
Status How it was reproduced: a PR: #46235 |
A response with no Content-Length and no chunked framing ends when the server closes the connection. The HTTP/1 client took every close for that end, so a TCP reset completed a truncated body as a whole 200 response. On Linux and Android the client now records how the read side ended: ReadEnd::Fin or ReadEnd::Error. The close-delimited arm of is_body_complete_on_close completes the body only on Fin. A body that a reset cut short fails with ConnectionClosed (ECONNRESET). The kernel gives the errno of a reset to the first call that meets it. The client keeps it in these cases: - Handler::on_close classifies the uSockets close code. It ignored the code before. - maybe_pause_receive does not read SO_ERROR under the rule. That read took the pending errno, and the next recv() returned 0 like a FIN. - A plain-TCP request write uses write_check_error. A send that fails with ECONNRESET sets a latch that turns Fin into Error. On TLS a close_notify from the peer ends its stream, whatever the connection does after it. A TLS connection that had a send rejected (us_socket_ssl_send_rejected) keeps the old rule: the code of its close stands for the lost records, not for a failed read. The rule does not apply to a 101-upgraded connection, a request over a unix socket, or a request through a CONNECT tunnel. On other systems fetch does what it did before.
fetch-gzip.test.ts has the fetch matrix, beside the close-delimited tests of the same function. A body with no Content-Length and no chunked framing fails with ECONNRESET when a reset cuts it short. The strict assertions are for Linux. On other systems the read must settle. The group "still the end of the body" passes with and without the rule. fetch-syscall-fault.test.ts has the deterministic cases. A fault puts the errno of the reset in one send() or recv(). http-reset-errno.test.ts is a source lint. It keeps two lists closed: the writes of the HTTP/1 client to its socket, and the reads of SO_ERROR in src/http. The fetch error section of the docs states the rule and its limits.
388e672 to
1d173c0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// How the read side of `socket` ended, from the close that uSockets reports with | ||
| /// `code`: above `CloseCode::fast_shutdown` that is the error of the failed read (loop.c). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The peer's close_notify ended its stream, whatever the connection did after | ||
| // it. BoringSSL sets the flag for a fatal read error too; uSockets closes that | ||
| // session itself, with no errno. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A send that the kernel rejected took the connection's error. The code of the | ||
| // close that follows stands for the lost records, not for a failed read, so it | ||
| // does not say how the read side ended: the body keeps the old rule. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A request write failed with ECONNRESET. The failed `send()` took that error from the | ||
| /// kernel, so the read side then ends with `recv() == 0` like a FIN. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// close-delimited response (no Content-Length, no Transfer-Encoding) whose | ||
| /// connection reached the end of the peer's stream. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether a close-delimited body completes only at the peer's FIN. Limited to the kernel whose | ||
| /// errno order was measured: on Linux a reset that follows the peer's FIN leaves EPIPE, and only | ||
| /// a reset with no FIN leaves ECONNRESET. Elsewhere every end of the connection completes such a | ||
| /// body. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether `errno`, a uSockets close code, says that the connection was cut before the peer's | ||
| /// stream ended. Up to `FastShutdown` a close code is this side's own close, and EPIPE is the | ||
| /// reset that followed the peer's FIN. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One write to the socket that carries a request. Returns what `socket.write` returns. | ||
| /// | ||
| /// A plain-TCP `send()` that fails with ECONNRESET takes the reset out of the kernel, so the | ||
| /// read side can no longer report it: `took_error` keeps that for `HTTPClient::on_close`. | ||
| /// ECONNRESET only: a send can also fail with a timeout or an abort after the peer's FIN | ||
| /// arrived, and the body is whole then. A TLS connection that had a send rejected keeps the | ||
| /// old rule (`Handler::read_end`). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // An upgraded connection is a byte stream with no body to cut. On a unix socket | ||
| // ECONNRESET only says that the peer closed with input unread, whole answer or not. | ||
| // Behind a CONNECT tunnel the socket ends with the proxy's teardown, not the origin's. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A close-delimited body ends at the peer's FIN (RFC 9112 section 8): a reset leaves | ||
| // it incomplete. undici completes it (`onHttpSocketError` in client-h1.js, | ||
| // nodejs/undici#784), so Node's fetch resolves with the part that arrived. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Under the rule, not SO_ERROR: reading it takes a pending reset out of the kernel, | ||
| // and the `recv()` after the pause then returns 0 like a FIN. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Reads AND clears `SO_ERROR`. Once it has taken a pending reset, the next | ||
| /// `recv()` returns 0 like a FIN, so a caller that still reads a body from | ||
| /// this socket must not call it: it is for the decision to keep or drop a | ||
| /// connection whose exchange is over. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Draft: two decisions needed.
Content-Length, no chunked framing) rejects when its connection ends without a FIN, even with every byte in. Node resolves it. No flag.int: the errno of a rejected TLS send.Problem
fetch()over HTTP/1.x completes a close-delimited body at any close. A TCP reset that cuts it resolves as a complete 200, andBun.writeleaves a short file.Handler::on_close(src/http/HTTPContext.rs) dropped the close code.Fix
ReadEnd). Only a FIN completes the body. A cut fails withECONNRESET, as for a framed body.fetch-gzip.test.ts,fetch-syscall-fault.test.ts, a source lint). 38 fail on main's source.Background
recv(),send()orSO_ERRORread. On LinuxEPIPEmeans the FIN came first.Downsides
Notes
The two questions
1. The difference from Node's fetch, with no flag. undici completes a close-delimited body on
ECONNRESETon purpose (onHttpSocketErrorinclient-h1.js, nodejs/undici#784, a macOS workaround for a whole body). curl exits 56 and Chrome errors the stream. After this PR, on Linux:close_notify) also rejects. The client cannot tell the two apart. Measured on this branch against a stock Node 26.3http.createServer(default header limit), 10 runs each,res.text()of Node's own 431 reply:ECONNRESET, 10ECONNRESET, 10ECONNRESET, 9fetch())fetch()itself still resolves with the status in these rows. Only the body read rejects.2. State in uSockets for fetch only.
int rejected_send_errorin the cold per-SSLus_ssl_rare_t(packages/bun-usockets/src/crypto/openssl.c, size stays 64 bytes, static assert added) and one accessor,us_socket_rejected_send_error.us_socket_tandus_socket_flagsdo not change.ECONNRESET(ssl_close_after_rejected_send). The errno of that send says what happened:EPIPEon Linux means the FIN came before the reset, so the body is whole. Without the int, a whole https answer to an upload that the server did not read rejects (the pinhttps: a FIN with no close_notify, then a resetfails). The cut-body tests pass without it.intin the padding ofus_socket_tafterunclassified_send_failures, stored byus_internal_classify_failed_send. It gives one home for plain TCP and TLS and removesInternalStateFlags::write_took_error. The review of usockets: drain the receive queue before closing on a peer reset #39860 asked to remove state fromus_socket_trather than add to it, so this PR does not take that path without a decision.write_check_erroron every TLS request write).us_ssl_rare_tand asserts 64 bytes. The two together are 72 bytes, so the second to land must agree on one layout.How the errno of a reset is kept
The kernel gives the errno of a reset to one caller. After that the next
recv()returns 0, like a FIN.recv()fails: uSockets closes with the errno as the close code (loop.c).Handler::read_endclassifies it. Before,on_closeignored the code.SO_ERROR:maybe_pause_receiveread it throughis_closed_or_has_error()before a receive pause. Under the rule it checksis_closed() || is_shutdown()only. Off Linux and Android the read stays.write_onceuseswrite_check_error. A send that fails withECONNRESETsetswrite_took_error, which turnsFinintoError.EPIPEand every other errno do not.close_notifyends its stream, whatever follows (SSL_RECEIVED_SHUTDOWNranks first inread_end).test/internal/source-lints/http-reset-errno.test.tskeeps two lists closed: onlywrite_oncewrites to the socket insrc/http/lib.rs, andSO_ERRORis read insrc/httponly where no body can still end.What still resolves as before
close_notify(browsers accept it).ECONNRESETonly says the peer closed with input unread.proxy, two Go relays). With the exemption 160 of 160 resolve.ECONNRESETthere. Nothing was run on those systems.Bun.fileupload over plain http (sendfile) is outside the rule.ECONNRESET(a timeout, an abort, or an errno uSockets retries).stream, socat in the self-review's runs).What else changes
ECONNRESET, not with the decoder's error that fetch: reject truncated compressed bodies on close-delimited responses #34922 documented. A cut on a gzip member or zstd frame boundary now rejects. It resolved before.the accepted break).ECONNRESETand the whole answer rejects (self-review run: 30 of 30 with the FIN dropped).bun installand the other users of the client get the same rule.Tests
test/js/web/fetch/fetch-gzip.test.ts: 43 new (30 fail on main's source, 13 pins that pass before and after). It holds the matrix beside the close-delimited tests of the same function.fetch.test.tsandfetch.tls.test.tsneed the network, so the before and after runs could not be done on them offline. That is why the tests are not there.test/js/web/fetch/fetch-syscall-fault.test.ts: 13 new (6 fail on main's source, 7 pins). They need fault injection.test/internal/source-lints/http-reset-errno.test.ts: 2.close_notifyrank, the unix exemption, the tunnel exemption, theSO_ERRORleg, the narrow send latch, and the accessor returning 0.chunked-trailing23/0,398461/0,fetch.unix16/0,fetch.upgrade1/0,fetch-keepalive49/0,fetch-redirect30/0,proxy103/0,serve-stream-body-error34/0.tls-syscall-fault(2 of 48, thebeside a stalled TLS sockettests),fetch-backpressure(5 of 129: four h3 tests and one keep-alive test),body(0, 1 and 2 of 778 in three runs, instring body consumption does not leak). The first two time out the same way on a debug build of main on the same machine.bodywas not compared with main.bun run rust:check-all: 12 ok.Not done
getsockopt(SO_ERROR)per receive pause (expected to go from about 1 per pause to 0 on Linux), instructions per plain-TCP request write throughus_socket_write_check_error, the TLS send path (the store is in anoinline, coldhelper),InternalStatesize, binary size.Self-review
30 concerns raised. 19 addressed in code, tests or text. 6 addressed in part (they name the two questions above and the systems that were not measured). Not taken: moving the tests to
fetch.test.tsandfetch.tls.test.ts(reason above). Left for the maintainer: the single store inus_socket_t(three entries, question 2). Open: the layout overlap with #46073.Prior work
on_closeblock.Bun.serveresets a cut HTTP/1.0 streamed body. This PR is the client side that hears such a reset.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-syscall-fault.test.ts, test/js/web/fetch/fetch-gzip.test.ts