fix: finish a socket write instead of dropping its tail - #689
Merged
Conversation
tcp_write and tcp_write_bytes are a single write(2). the kernel takes what fits in the send buffer and returns that count; the rest is not queued anywhere. every std caller discarded the count, so a buffer larger than the send buffer lost its tail silently and the peer waited for bytes that were never written. a 1 MiB redis SET worked; a 4 MiB one hung. std.net.tcp grows write_all and write_all_bytes, which resume from where the last write stopped and only give up when a write accepts nothing at all — that is the reader having gone, and it is the only condition that ends the loop early. redis, postgres, mysql, the h2c client preface and both http/2 plaintext transports now write through it. the loop runs on bytes rather than on text, which turned out to matter more than the missing loop did. std.io already looped correctly, but it resumed by re-slicing the String, and a send buffer fills at whatever byte offset it fills at — including the middle of a multi-byte character, which a String cannot be cut at. TcpStream.write_all on three megabytes of non-ascii stopped the process outright. the text write_all helpers now encode once and resume through their bytes counterpart. the tls write path was already right: Conn.write_bytes caps at one 16 KiB record, but Conn.write_all_bytes loops over it and its socket writes go through std.io's byte loop. separately, the redis reply parser read line[0] to get the tag with nothing checking the line was non-empty. read_line returns what came before the terminator, so a bare CRLF handed it "", and indexing a String is strict — a peer sending two bytes killed the client process, while every other malformed shape in the same parser came back as an ordinary error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tcp_writeandtcp_write_bytesare a singlewrite(2). The kernel takes what fits in the socket send buffer and returns that count; the rest is not queued anywhere. Acrossstd/no caller looped —std/postgres.pithwas the only site that even captured the count, and only to checkis_err. So a buffer larger than the send buffer lost its tail silently and the peer waited for bytes that were never written. That is the reported redis symptom: a 1 MiBSETworks, a 4 MiB one hangs.std.net.tcpgainswrite_allandwrite_all_bytes. A short write is treated as normal rather than as an error — the loop resumes from where the last one stopped — and the only condition that ends it early is a write that accepts nothing at all, which means the reader is gone. That matches whatfdio::write_channelin the runtime already promised: it returns0only for a real close, and waits on the reactor rather than returning0for a would-block. This was a std-layer omission, not a runtime one, so nothing undercranelift/changed.Adopted at every site that wrote straight to the builtins:
std/redis.pith,std/postgres.pith(both the framed send path and the pre-TLS SSLRequest),std/mysql.pith, the h2c client preface instd/web.pith, and the plaintext transports instd/net/http2/connection.pithandstd/net/http2/server.pith.std/net/tcp.pith's ownwritestays a single syscall and now documents that loudly, since the count is the whole result and a caller may legitimately want it.Looping on bytes rather than on text turned out to matter more than the missing loop did.
std/io.pithalready looped correctly, but it resumed by re-slicing theString. A send buffer fills at whatever byte offset it fills at, including the middle of a multi-byte character, and aStringcannot be cut there —substringat a non-boundary offset stops the process.TcpStream.write_allon three megabytes of non-ascii died outright, and the buffered TCP writer flushes through that same helper. The textwrite_allhelpers now encode once and resume through their bytes counterpart.The TLS write path was already correct and is unchanged.
Conn.write_bytescaps at one 16 KiB record, so it is a partial write by construction, butConn.write_all_bytesloops over it and its socket writes go throughstd.io's byte loop.Separately, the redis reply parser read
line[0]for the reply tag with nothing checking the line was non-empty.read_linereturns what came before the terminator, so a bare CRLF handed it"", and indexing aStringis strict: a peer sending two bytes killed the client process. Every other malformed shape in that parser — unknown tag, bad length, truncated payload, exceeded limit — comes back as an ordinary error, and now this one does too.what was tested
The 4 MiB hang was reproduced first. A stub server that accepts and then drains slower than the client fills gives a short write of 2,625,195 of 4,194,304 bytes under
PITH_GREEN=1, and the redisSETon top of it times out at 60s (exit 124). UnderPITH_GREEN=0the same case completes, because that backend's socket is blocking — the bug only bites on green, which is the default.Two regression cases were added, and each fix was checked by breaking it again:
tests/cases/test_tcp_write_all.pithsends 4 MiB of ascii and 3 MiB of three-byte characters to a deliberately slow reader, through the fd helper, throughTcpStream.write_all, and through a redisSET. Reverting the redis call to a baretcp_writemakes it time out at exit 124. Reverting the helper's resume to a single write makes the first case hang. Restoringstd/io.pith's string-based resume makes it die withsubstring(2672619, 3145729) would split the character '€'. The non-ascii payload carries a one-byte ascii prefix so the first short write cannot land on a character start.tests/cases/test_redis_bare_crlf.pithdrives a stub that replies with a bare CRLF, a lone LF, and an empty line nested inside an array. Each returns an error, and the process is still running at the end to say so. With the guard removed it dies withstring index out of bounds: 0 for string of length 0, exit 1.make run-regressions-onlyis 331/331, up from 329 by the two new cases. Both new cases produce byte-identical output underPITH_GREEN=1,PITH_GREEN=1 PITH_GREEN_WORKERS=1andPITH_GREEN=0, so the corpus verification under both backends covers them.make memcheckis clean, and both new cases were additionally run under valgrind directly, since they touch the socket path and the redis parser. The four edited examples run, andweb_h2andredis_clientstill match their expected output.