Siyao Meng created HDDS-17443:
---------------------------------
Summary: Domain socket Receiver logs an NPE for every response
without a block file and sends no reply to a GetBlock that is not granted access
Key: HDDS-17443
URL: https://issues.apache.org/jira/browse/HDDS-17443
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
CR-1-reply-to-getblock-without-block-file-in-domain-socket-receiver.patch,
TestBugCR1NullBlockFileClose.java, TestBugMC2GetBlockWithoutFlagNoResponse.java
h3. Mechanism
{{Receiver.ProcessRequestTask.run}} attaches a block file to the response only
for a GetBlock that returns SUCCESS, and both places that use the file assume
this is the same as "the response has a file":
* {{Receiver.sendResponse}} closes {{entry.getFile()}} in its {{finally}} block
without a null check, and catches only {{IOException}}. For every response
without a file (Echo, a GetBlock that does not succeed, the UNSUPPORTED_REQUEST
reply) the close throws a {{NullPointerException}} after the response has been
flushed. The exception skips {{ContainerMetrics.incContainerLocalOpsLatencies}}
and is logged by {{ProcessRequestTask.run}} as "Failed to processRequest" at
ERROR with the stack trace.
* {{KeyValueHandler.handleGetBlock}} opens and parks a block file only when the
request sets {{requestShortCircuitAccess}} and the domain socket server is
started, and returns SUCCESS with {{shortCircuitAccessGranted=false}}
otherwise. {{ProcessRequestTask.run}} fetches the file for every SUCCESS
GetBlock, {{Objects.requireNonNull}} throws, the exception is only logged, and
no response is sent.
HDDS-11627 closed the block's stream only when there was one; HDDS-12883
brought the close back for the block file without that null check. The non-null
requirement on the file dates from HDDS-11627.
h3. Trigger
* Logged NPE: every Echo, and every short-circuit GetBlock that does not
succeed. The most frequent one is the routine fallback when the local datanode
has not applied the PutBlock yet ({{UNKNOWN_BCSID}}, see the comment in
{{BlockInputStream.getBlockDataUsingClient}}). Run for {{UNKNOWN_BCSID}} and
{{NO_SUCH_BLOCK}}; other error results such as {{CONTAINER_NOT_FOUND}} or a
token failure take the same path.
* No reply: a GetBlock on the domain socket without
{{requestShortCircuitAccess}}. The field is optional and defaults to false, so
this is a valid request, but the in-tree reader
({{BlockInputStream.getBlockDataUsingSCClient}}) always sets it. By code
reading the same happens when the domain socket server is stopped between the
block lookup and the {{isStarted()}} check (not run); there {{closeAllPeers}}
disconnects the client anyway.
h3. Impact
* Every Echo and every failed short-circuit GetBlock adds an ERROR with a stack
trace to the datanode log, and {{localLatencyNsEcho}} and
{{localLatencyNsGetBlock}} miss those samples ({{numLocalEcho}} and
{{numLocalGetBlock}} are correct). The client receives a correct response and
falls back to gRPC as designed. Reproduced. No in-tree reader sends Echo on the
domain socket (the Echo load generator in freon uses gRPC), so in practice the
errors come from failed GetBlocks.
* A client that sends a GetBlock without the flag gets no reply and waits for
its own read timeout. Only that request is affected: the read thread goes back
to the pool and the same connection serves the next request normally
(reproduced with a single read thread). Other connections and clients are not
affected. No in-tree client sends such a request, so today this only matters
for other clients of the protocol.
h3. Reproduction
[^TestBugCR1NullBlockFileClose.java] and
[^TestBugMC2GetBlockWithoutFlagNoResponse.java]
({{hadoop-ozone/integration-test}}) run a real {{XceiverServerDomainSocket}}
with {{HddsDispatcher}} and {{KeyValueHandler}} and send requests framed as the
short-circuit client frames them. They need the native libhadoop
({{-Phadoop-native-lib}}) and change no source. Both pass on unmodified source,
so a passing test means the defect is present.
* The first sends an Echo, a GetBlock with BCSID 5 against a replica at 0
({{UNKNOWN_BCSID}}) and a GetBlock of a missing block ({{NO_SUCH_BLOCK}}). All
three get the right response, and the datanode logs three
{{NullPointerException: Cannot invoke "java.io.RandomAccessFile.close()"}}
errors. {{numLocalGetBlock}} is 3 with 1 latency sample (the granted control
request) and {{numLocalEcho}} is 1 with 0 samples.
* The second sends a GetBlock with the flag (answered with its descriptor),
then the same GetBlock without the flag: no reply within the 3 s read timeout,
and the datanode logs {{NullPointerException: Failed to get block file for
block}}. A third GetBlock on the same connection is answered normally.
h3. Patch
[^CR-1-reply-to-getblock-without-block-file-in-domain-socket-receiver.patch],
against 69d77beff60b9106e2517c36a7a1b2cc726af568. It also applies to master at
1acf315d546.
Two changes in {{Receiver}}: {{sendResponse}} closes the block file only when
there is one, and {{ProcessRequestTask.run}} fetches the parked file only when
the GetBlock response grants short-circuit access. A SUCCESS GetBlock that
grants none is then sent as the handler built it, with its BlockData and
without a descriptor, and by code reading the in-tree client takes its existing
"Short-circuit access is denied" path (fails the request, disables
short-circuit for the configured interval) and reads through gRPC.
{{KeyValueHandler.handleGetBlock}} grants access exactly when it parks a file,
so the handler is unchanged. The two changes go together: with only the second
one, the reply that is now sent would hit the first problem right after it is
flushed. The new tests pass the read executor argument of the
{{XceiverServerDomainSocket}} constructor like the existing tests; after the
change in HDDS-17442, which removes that argument, they drop it as well.
Covered by {{testResponseWithoutBlockFile}} and
{{testGetBlockWithoutShortCircuitAccess}} in the existing
{{TestXceiverServerDomainSocket}}. Without the change the first fails because
the Receiver logged "Failed to processRequest" for the Echo and the failed
GetBlock, and the second fails with {{SocketTimeoutException}}. With it
{{TestXceiverServerDomainSocket}}, {{TestXceiverClientManagerSC}},
{{TestLocalChunkInputStream}} and {{TestDomainSocketFactory}} (19 tests) and
{{TestKeyValueHandler}} (43 tests) pass, checkstyle is clean, and both
reproductions fail because the error is gone and the reply arrives.
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-12883 (added the close of the
block file after it is sent), HDDS-16918 (fallback to gRPC is not logged on the
datanode), 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]
