yandrey321 commented on code in PR #11783:
URL: https://github.com/apache/ozone/pull/11783#discussion_r4426991132
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -149,6 +149,8 @@ public synchronized void initialize() throws IOException {
if (initialized) {
return;
}
+ // A closed stream has no client to get the block with.
+ checkOpen();
Review Comment:
It isn't initialized *again*. It's a stream that was closed *before its
first read*, so it was never initialized.
`close()` only releases the client and sets `xceiverClientFactory = null`,
which is what `checkOpen()` tests; it
doesn't touch `initialized`. A stream closed before its first read still has
`initialized == false`, and its first
read goes into `initialize()`. A stream closed after initialization returns
early from `initialize()`, and the
caller's own `checkOpen()` catches it, as you say.
With this PR that can happen during a concurrent close:
1. `ECBlockInputStream.readPositioned` takes the per-index stream under
`streamLock` (`getOrOpenStream`). That opens it
lazily, uninitialized, and the read task then runs outside the lock.
2. Another thread calls `close()`, which runs `closeStreams()` →
`BlockInputStream.close()`.
3. The read task calls `stream.read(pos, buf)` → `readPositioned` →
`!initialized` → `initialize()`.
Without the new check, `getBlockData()` → `acquireClient()` silently does
nothing (the factory is null), and
`getBlockDataUsingGRPCClient(null, …)` throws an NPE. That comes out of
`initialize()` before the caller's
`checkOpen()`, and the retry loop catches only `IOException`. The new check
turns it into the usual
"BlockInputStream has been closed." `IOException`.
`testPositionedReadOfClosedUninitializedStreamFailsWithIOException`
covers it.
Before this PR, each positioned read used its own per-call clone, which
nothing else could close, so this window
didn't exist. The same check also covers an empty block: it never sets
`initialized`, so every read goes through
`initialize()` again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
