Repository navigation
KAFKA-21280: Do not send stale bytes after a failed file read in SslTransportLayer.transferFrom - #23832
Conversation
…ransportLayer.transferFrom transferFrom clears fileChannelBuffer before reading the next chunk of the file. When the read threw, or returned no bytes, the buffer was left cleared rather than flipped, so it still looked full of the previous chunk; when bytes had already been written in the same call the IOException was swallowed, and the next call sent the stale buffer as the contents of the position that could not be read. A read error on the segment never surfaced and the client received a corrupt batch. Flip the buffer whether or not the read succeeds, so that it holds exactly what was read: nothing after a failed read. The next call then reads again and the original IOException propagates. Generated-by: Claude (Anthropic)
|
Updated for 854779b. 868d825 rethrows a failed read even when part of the transfer was already written, and Two tests in End to end, this PR at 868d825: one controller and three brokers in Docker on a Linux 6.8 host, the leader's log
With this PR alone the read error surfaces as it does over PLAINTEXT (each failed send logs it and closes the Ran the full |
| int bytesRead; | ||
| try { | ||
| bytesRead = fileChannel.read(fileChannelBuffer, pos); | ||
| } finally { |
There was a problem hiding this comment.
The following code seems to be copied from the JDK implementation. In Kafka's usage, we typically close a connection on IOException, it seems that we can just rethrow on IOException without returning totalBytesWritten.
} catch (IOException e) {
if (totalBytesWritten > 0)
return totalBytesWritten;
throw e;
}
There was a problem hiding this comment.
Done: transferFrom now rethrows. I kept the flip, since a read that returns -1, e.g. after a concurrent truncation, throws nothing, and without the flip the buffer would still look full of the previous read on the next call. FileRecords.writeTo fails that next call with its truncation check unless the file has grown back past the slice by then, but transferFrom should not rely on its caller for that; testTransferFromDoesNotSendStaleBytesAtEndOfFile covers it.
An IOException from transferFrom closes the connection, so returning the bytes already written only delayed the error to the next call. Rethrow it instead. The buffer is still flipped after a failed read, and after a read at the end of the file, which throws nothing. Generated-by: Claude (Anthropic)
|
Thanks, Jun. Addressed in 868d825; the description and the test notes are updated. |
| assertThrows(IOException.class, () -> layer.transferFrom(fileChannel, 0, 3 * TRANSFER_SIZE)); | ||
| // A transfer from the unreadable position must fail as well instead of sending what was read before | ||
| assertThrows(IOException.class, () -> layer.transferFrom(fileChannel, TRANSFER_SIZE, 2 * TRANSFER_SIZE)); | ||
| assertArrayEquals(fileContents(TRANSFER_SIZE), sent.toByteArray()); |
There was a problem hiding this comment.
Should we add this check after the first transferFrom() too? Ditto in the test below.
There was a problem hiding this comment.
Done, in both tests.
| int bytesRead = fileChannel.read(fileChannelBuffer, pos); | ||
| if (bytesRead <= 0) | ||
| break; | ||
| while (totalBytesWritten < totalBytesToWrite) { |
There was a problem hiding this comment.
Could we add a javadoc that transferFrom() throws IOException, after which, the channel shouldn't be used?
… failed transferFrom Also check what was sent after the first transferFrom call in both tests. Generated-by: Claude (Anthropic)
|
Thanks, Jun. Addressed in 854779b; the test notes are updated. |
…py Fetch path (#23445) The Fetch response is sent zero copy: the records are read from the segment by `FileRecords.writeTo`, on the network thread, while the response is written to the socket. A failed read of the medium and a client that went away both arrive there as a bare `IOException`, and `Selector` treats every one of them as a disconnect. A disk error on this path therefore never offlines the log directory, unlike an I/O error in the `LocalLog` operations. This change offlines it, with the same split as elsewhere in the log: the layer that sees the `IOException` reports it, and storage decides. * `LocalLog.read` gives the `FileRecords` slices it returns a transfer failure handler. * When the transfer fails, `FileRecords.writeTo` passes the region it was writing to the handler and rethrows the original exception unchanged. A `ClosedChannelException` is not passed on: the file was not read. * The handler re-reads the region on the log's scheduler, at most once at a time per log directory, and calls `LogDirFailureChannel.maybeAddOfflineLogDir` only if the read fails, with the failed transfer attached. As the log cleaner does, the directory is offlined and the error is not rethrown. The network thread does no extra I/O on a failed send. The send itself is logged as a disconnect, at DEBUG; the ERROR comes from `LogDirFailureChannel`. The classification is as accurate as what the JVM reports, and no more. Nothing below the JVM is second-guessed: a transient media error that has cleared by the time of the re-read is attributed to the socket, and a read that fails for a reason other than the medium is treated as a storage failure, the criterion `LocalLog` already applies. At the JVM level, the re-read of the region: * reads completely: the socket was at fault; * reaches the end of the file: the log was truncated concurrently (e.g. the replica became a follower); * makes no progress: nothing can be concluded; * fails with `ClosedChannelException` or a subclass: the segment was closed or deleted concurrently, which says nothing about the medium (the reasoning of KAFKA-19571); * fails with any other `IOException`: the log directory is offlined. The whole region is re-read rather than its first byte because, depending on the platform, the transfer can fail anywhere within it without reporting progress: on macOS both `sendfile(2)` and `read(2)` drop the partial progress of a chunk whose read fails, and on Windows the JDK does not use `TransmitFile` for a non-blocking socket and falls back to a mapped buffer or `ReadFile`. On Linux the transfer fails at the first unreadable page. The re-read reads at most what the failed call was about to send. The message text of the exception is not inspected: it is platform specific. Once the directory is offline its segments are closed, and the next Fetch gets `KAFKA_STORAGE_ERROR`. Consequences worth stating: * A broker whose only log directory, or whose metadata log directory, fails on this path now shuts down, as it does today for any other I/O error in that directory. * The KRaft metadata log is read through `LocalLog` too. A read failure while the leader sends it marks the raft log's directory offline, the next append on the leader fails, and the raft I/O thread's fatal fault handler stops the node, as it does today for an I/O error in `LocalLog.read` on the metadata log. * The checks run on the broker's shared background scheduler (the raft manager's own scheduler for the metadata log). * KRaft snapshots are sent through `UnalignedFileRecords`, which this change does not cover; I will follow up separately. * On TLS listeners a read error does not reach `FileRecords` today: `SslTransportLayer.transferFrom` swallows it and sends a stale buffer instead (KAFKA-21280, #23832). This change takes effect on TLS listeners once that is fixed. Generated-by: Claude (Anthropic) Reviewers: Jun Rao <junrao@gmail.com>
2 participants
SslTransportLayer.transferFromclearsfileChannelBufferbeforereading the next chunk of the file. When the read threw, or returned no
bytes, the buffer was left cleared rather than flipped, so it still
looked full of the previous chunk. If bytes had already been written in
the same call the
IOExceptionwas swallowed, and the next call sentthe stale buffer as the contents of the position that could not be read:
a read error on the segment never surfaced, and the client received a
corrupt batch.
This flips the buffer whether or not the read succeeds, so that it holds
exactly what was read: nothing after a failed read or at the end of the
file. It also rethrows the
IOExceptionwhen bytes were already writtenin the same call: the connection is closed on it anyway, and returning
the bytes written only delayed the error to the next call.
Generated-by: Claude (Anthropic)
Reviewers: Jun Rao junrao@gmail.com