Siyao Meng created HDDS-17444:
---------------------------------
Summary: Domain socket Receiver keeps the connection after a
response send fails partway, so the next response follows a cut off frame
Key: HDDS-17444
URL: https://issues.apache.org/jira/browse/HDDS-17444
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-8-close-domain-socket-connection-after-failed-response-send.patch,
TestBugMC8PartialResponseKeepsConnection.java
h3. Mechanism
{{Receiver.sendResponse}} writes the version, the command type, the
length-prefixed response and, for a granted GetBlock, the file descriptor. If
any of these writes fails, for example on the send timeout
({{ozone.client.write.timeout}}, 30 s by default) after part of the response is
on the wire, its {{catch (Throwable)}} only logs "Failed to send response" and
closes the block file. The connection stays open and the read loop keeps
handling requests, so the next response is written right after the cut off one.
The client ({{XceiverClientShortCircuit.ReceiveResponseTask}}) reads the length
the first frame declared and fills it with the bytes that follow, then matches
the parsed message by its clientId and callId, which are the last fields of the
message. By code reading the same happens when the descriptor send fails after
the response was flushed: the client then takes the first byte of the next
frame as the descriptor byte and the stream is out of step by one byte (not
run).
The log and continue handling dates from HDDS-11627.
h3. Trigger
Responses are pending on a connection and the client does not read for the
whole send timeout, so the datanode's socket buffer fills and a send times out
partway. The client's reader is a dedicated thread, so this needs the client's
receive thread to stall for that long. By default the client's own request
timeout ({{ozone.client.read.timeout}}, 30 s) equals the datanode's send
timeout, so the request whose response is cut has usually already failed on the
client when the send fails; a later request completes from the stream only if
it was sent while the receive thread alone was stalled.
h3. Impact
* Shown on a real {{XceiverServerDomainSocket}}: after a send timeout the
connection stays open, and the bytes on it are the first part of a 4 MB
response (8192 bytes on macOS with the default socket buffer) followed by the
complete next response.
* By code reading, the client then parses the next bytes as the rest of the
cut-off frame. If too few bytes follow, its read blocks until
{{ozone.client.read.timeout}} and then fails every pending request. Otherwise
the merged bytes are parsed as one message, which usually fails to parse or
carries a callId the client no longer waits for, and then the receive loop also
fails every pending request. Both outcomes end in the gRPC fallback. Which one
occurs depends on where the cut falls. In a run with the real
{{XceiverClientShortCircuit}} (receive thread held until the datanode logged
the failed send, three Echo requests pending, not attached), all three requests
stayed pending until their request timers fired, and the client still treated
the connection as open.
* By code reading, not run: when the cut-off bytes parse as one message for the
same block, protobuf merges the two chunk lists, and since {{BlockInputStream}}
computes chunk positions from chunk lengths, a read bound to that descriptor
could return bytes that do not belong at that offset. This was not shown.
Either way, the framing on the connection can no longer be trusted.
h3. Reproduction
[^TestBugMC8PartialResponseKeepsConnection.java]
({{hadoop-ozone/integration-test}}) runs a real {{XceiverServerDomainSocket}}
with {{HddsDispatcher}} and {{KeyValueHandler}}, a 1 s send timeout and the
native libhadoop ({{-Phadoop-native-lib}}), and changes no source. It sends two
Echo requests on one connection, the first asking for a 4 MB response, and
reads nothing until the datanode has logged the failed send. The client's read
then times out on the open connection, and the bytes it received are the first
part of the first response (8192 of 4194341 bytes on macOS with the default
socket buffer) followed by the second response, compared byte for byte. Echo
with a large response only makes the send timeout deterministic; GetBlock
responses and descriptors use the same write path. The test passes on
unmodified source, so a passing test means the defect is present.
h3. Patch
[^MC-8-close-domain-socket-connection-after-failed-response-send.patch],
against 69d77beff60b9106e2517c36a7a1b2cc726af568. It also applies to master at
1acf315d546.
When a send fails, {{sendResponse}} now closes the peer after logging, while it
still holds the connection's send lock, so no other response can follow the
partial one. The read loop then ends through its normal path and
{{XceiverServerDomainSocket.closePeer}} releases the connection once, as
before. Responses still queued for that connection fail at once and close their
block files. On the client, the receive loop fails on the truncated message and
fails every pending request at once, as it does when the datanode closes an
idle connection, and the reads fall back to gRPC. In the client run described
under Impact, with the patch the three pending requests failed within a few
milliseconds with {{InvalidProtocolBufferException}} (the input ended in the
middle of a field) through the receive loop's generic {{catch}}, and a later
request failed with "... is not open". Nothing changes while sends succeed. A
client side check cannot make a stream with a partial frame parseable again, so
closing is the repair.
Covered by {{testConnectionClosedAfterFailedResponse}} in the existing
{{TestXceiverServerDomainSocket}}: without the change the client's read times
out on the open connection ({{SocketTimeoutException}}), with it the client
receives part of the first response and then the end of the stream, and the
second response is not among the received bytes. This patch and the patch of
the separate issue HDDS-17443 add the same test helpers ({{sendRequest}},
{{getEchoRequest}}) to {{TestXceiverServerDomainSocket}}, so whichever is
applied second needs a trivial rebase that drops the duplicate. The new test
passes the read executor argument of the {{XceiverServerDomainSocket}}
constructor like the existing tests; after the change in HDDS-17442, which
removes that argument, it drops it as well. With the change
{{TestXceiverServerDomainSocket}}, {{TestXceiverClientManagerSC}},
{{TestLocalChunkInputStream}} and {{TestDomainSocketFactory}} (18 tests) and
{{TestKeyValueHandler}} (43 tests) pass, checkstyle is clean, and the
reproduction fails because the stream ends instead.
Found by TLA+ model checking and code review of the datanode short-circuit read
paths under HDDS-15926, on commit 69d77beff60b9106e2517c36a7a1b2cc726af568.
Checked against HDDS issues and apache/ozone pull requests for duplicates
before filing. Related but not duplicates: HDDS-11627 (added GetBlock on the
short-circuit channel and this send path), HDDS-16335 (concurrent access on the
client side of the domain socket). The attached patch is a proposal for review.
Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
