Skip to content

HDDS-15422. Stream read seek should not close stream #10415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
szetszwo merged 10 commits into from
Aug 14, 2026
Merged

HDDS-15422. Stream read seek should not close stream #10415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3479dd8
HDDS-15422. Stream read seek should not close stream
szetszwo Jun 2, 2026
8a37e8b
Put back the proto to queue for seek.
szetszwo Jun 2, 2026
541bf94
Fix checkstyle
szetszwo Jun 2, 2026
750c5ac
Check readBuffer == null
szetszwo Jun 2, 2026
70d7648
Reuse read buffer
szetszwo Jun 5, 2026
ab6302b
Address review comments.
szetszwo Jun 8, 2026
cef6b47
Fix the bugs discovered by Cursor.
szetszwo Jun 17, 2026
5aaa708
Merge branch 'am' into HDDS-15422
szetszwo Jun 17, 2026
78d4668
Merge branch 'am' into HDDS-15422
szetszwo Jul 24, 2026
ecd5968
Merge branch 'am' into HDDS-15422
szetszwo Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...op-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/MultipartInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,9 @@ public synchronized void seek(long pos) throws IOException {
}

// Reset the previous partStream's position
partStreams.get(prevPartIndex).seek(0);
if (prevPartIndex != partIndex) {
partStreams.get(prevPartIndex).seek(0);
}

// Reset all the partStreams above the partIndex. We do this to reset
// any previous reads which might have updated the higher part
Expand Down
186 changes: 113 additions & 73 deletions ...-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public class StreamBlockInputStream extends BlockExtendedInputStream {
private XceiverClientFactory xceiverClientFactory;
private XceiverClientGrpc xceiverClient;

private ByteBuffer buffer;
private ReadBuffer readBuffer;
private long position = 0;
private long requestedLength = 0;
private StreamingReader streamingReader;
Expand Down Expand Up @@ -116,6 +116,8 @@ public StreamBlockInputStream(
this.responseDataSize = config.getStreamReadResponseDataSize();
this.readTimeout = config.getStreamReadTimeout();
this.readTimeoutNanos = readTimeout.toNanos();

LOG.debug("{}: new StreamBlockInputStream", name);
}

@Override
Expand All @@ -136,11 +138,12 @@ public synchronized long getPos() {
@Override
public synchronized int read() throws IOException {
checkOpen();
if (!dataAvailableToRead(1, true)) {
final boolean preRead = true;
if (!dataAvailableToRead(1, preRead)) {
return EOF;
}
int value = buffer.get();
advancePosition(1);
final int value = readBuffer.getByteBuffer().get();
advancePosition(1, preRead);
return value;
}

Expand All @@ -162,12 +165,14 @@ synchronized int readFully(ByteBuffer targetBuf, boolean preRead) throws IOExcep
if (!dataAvailableToRead(targetBuf.remaining(), preRead)) {
break;
}

final ByteBuffer buffer = readBuffer.getByteBuffer();
int toCopy = Math.min(buffer.remaining(), targetBuf.remaining());
ByteBuffer tmpBuf = buffer.duplicate();
tmpBuf.limit(tmpBuf.position() + toCopy);
targetBuf.put(tmpBuf);
buffer.position(tmpBuf.position());
advancePosition(toCopy);
advancePosition(toCopy, preRead);
read += toCopy;
}
return read > 0 ? read : EOF;
Expand All @@ -177,30 +182,31 @@ private synchronized boolean dataAvailableToRead(int length, boolean preRead) th
if (position >= blockLength) {
return false;
}

while (true) {
try {
initialize();
if (bufferHasRemaining()) {
return true;
if (!hasRemaining(readBuffer)) {
readBuffer = streamingReader.read(length, preRead);
}
buffer = streamingReader.read(length, preRead);
retries = 0;
return bufferHasRemaining();
return hasRemaining(readBuffer);
} catch (IOException ex) {
handleExceptions(ex);
}
}
}

private synchronized void advancePosition(long delta) {
private synchronized void advancePosition(long delta, boolean preRead) {
LOG.trace("{}: advance {} -> {}", getName(streamingReader), position, position + delta);
position += delta;
if (position >= blockLength && streamingReader != null) {
closeStream();
if (preRead && position >= blockLength) {

@sodonnel sodonnel Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't changed as part of this PR, but why do we only close the reader if preRead is true?

@szetszwo szetszwo Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preRead:

  • true for sequential read
  • false for position read

For position read, we should keep the reader running.

closeReader("advancePosition");
}
}

private synchronized boolean bufferHasRemaining() {
return buffer != null && buffer.hasRemaining();
private static boolean hasRemaining(ReadBuffer read) {
return read != null && read.getByteBuffer().hasRemaining();
}

@Override
Expand All @@ -220,10 +226,42 @@ public synchronized void seek(long pos) throws IOException {
if (pos == position) {
return;
}
LOG.debug("{}: seek {} -> {}", this, position, pos);
closeStream();
LOG.debug("{}: seek {} -> {}", getName(streamingReader), position, pos);
readBuffer = reuseReadBuffer(readBuffer, pos);

@sodonnel sodonnel Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets say we have 32MB pre-read in the queue and we are within the first 1MB. Then we seek to 10MB offset. reuseReadBuffer will return null.

Then later in read() we call dataAvailableToRead which triggers a call streamingReader.read() if there is no remaining in the buffer or the buffer is null.

  private synchronized boolean dataAvailableToRead(int length, boolean preRead) throws IOException {
    if (position >= blockLength) {
      return false;
    }
    initialize();

    if (!hasRemaining(readBuffer)) {
      readBuffer = streamingReader.read(length, preRead);
    }
    Preconditions.assertTrue(hasRemaining(readBuffer));
    return true;
  }

Inside streamingReadder.read:

 private ReadBuffer read(int length, boolean preRead) throws IOException {
      checkError();
      if (future.isDone()) {
        return null; // Stream ended
      }

      readBlock(length, preRead);  // !! Reads more data before polling

      while (true) {
        final ReadBlockResponseProto proto = poll();

It pre-reads more data duplicating data already on the queue and also pulling more data onto the queue that the pre-read limit.

This is the sort of thing the boundary tests for 'reading within the pre-read doesn't issue new read calls' would catch.

@sodonnel sodonnel Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've attached a diff that has a unit test to validate that we don't re-read data already on the queue when seeking forward. I tried to create a similar test for a backwards seek but its more difficult as we want to test that the queue is drained before requesting more data, which isn't really possible with the current structure.

But I think we need to change the reading behavior so it reads through the queue and only reads more data from the datanode if the queue has been drained.

In a dysfunctional case, we could have read 32MB onto the queue with pre-read. Seek from 0.1MB to 1.1MB. This would trigger another 32MB read onto the end of the queue, but the data we want is there are the head. Now there is 31 + 32MB on the queue. Seek now to 3.1MB, the same happens, as we have the next chunk we need available.

Even with the backwards seek, we could have 32MB on the queue, and then read 32MB more before throwing away the first 32MB. We should throw away the first 32MB first.

forward-seek-test.patch

position = pos;
requestedLength = pos;
if (readBuffer == null) {
// Only rewind the request high-watermark when the buffered (already requested/served) data cannot be reused;
// otherwise we would re-request data that is still buffered.
requestedLength = pos;
}
}

static ReadBuffer reuseReadBuffer(ReadBuffer previous, long blockOffset) {
if (previous != null) {
final ByteBuffer buffer = getByteBuffer(previous.getProto(), blockOffset);
if (buffer != null && buffer.hasRemaining()) {
previous.getByteBuffer().position(buffer.position());
Preconditions.assertSame(buffer.remaining(), previous.getByteBuffer().remaining(), "remaining");
return previous;
}
}
return null;
}

static ByteBuffer getByteBuffer(ReadBlockResponseProto proto, long blockOffset) {
final ByteBuffer buffer = proto.getData().asReadOnlyByteBuffer();
// Adjust buffer position since the server always returns data starting at checksum boundary.
final long protoOffset = proto.getOffset();
if (blockOffset < protoOffset) {
// This can happen after seek, just drop it for now
// TODO: consider to cache the proto, which will be useful when seeking back.
return null;
}
final long offset = blockOffset - protoOffset;
if (offset > 0) {
buffer.position(Math.toIntExact(Math.min(offset, buffer.limit())));
}
return buffer;
}

@Override
Expand All @@ -238,19 +276,15 @@ public synchronized void unbuffer() {
releaseClient();
}

private synchronized void closeStream() {
private synchronized void closeReader(String reason) {
readBuffer = null;
if (streamingReader == null) {
buffer = null;
return;
}

final StreamingReader reader = streamingReader;
streamingReader = null;
buffer = null;

if (LOG.isDebugEnabled()) {
LOG.debug("Closing {}", reader);
}
LOG.debug("{} closeReader for {}", getName(reader), reason);

reader.onCompleted();

Expand Down Expand Up @@ -305,6 +339,7 @@ private synchronized void initialize() throws IOException {
try {
acquireClient();
final StreamingReader reader = new StreamingReader();
LOG.debug("{}: new StreamingReader", getName(reader));
xceiverClient.initStreamRead(blockID, reader, failedStreamingDatanodes);
streamingReader = reader;
} catch (IOException ioe) {
Expand Down Expand Up @@ -373,7 +408,7 @@ private void recordFailedStreamingDatanode() {

protected synchronized void releaseClient() {
if (xceiverClientFactory != null && xceiverClient != null) {
closeStream();
closeReader("releaseClient");
xceiverClientFactory.releaseClientForReadData(xceiverClient, false);
xceiverClient = null;
}
Expand Down Expand Up @@ -413,6 +448,35 @@ public Duration getReadTimeout() {
return readTimeout;
}

private Object getName(StreamingReader reader) {
return reader != null ? reader : name;
}

static class ReadBuffer {
private final ReadBlockResponseProto proto;
private final ByteBuffer buffer;

ReadBuffer(ReadBlockResponseProto proto, ByteBuffer buffer) {
this.proto = proto;
this.buffer = buffer;
}

ReadBlockResponseProto getProto() {
return proto;
}

ByteBuffer getByteBuffer() {
return buffer;
}

@Override
public String toString() {
return "ReadBuffer: offset=" + proto.getOffset()
+ ", dataSize=" + proto.getData().size()
+ ", buffer=" + buffer;
}
}

/**
* Implementation of a StreamObserver used to received and buffer streaming GRPC reads.
*/
Expand Down Expand Up @@ -462,61 +526,43 @@ ReadBlockResponseProto poll() throws IOException {
}

final long elapsedNanos = System.nanoTime() - startTime;
if (elapsedNanos >= readTimeoutNanos) {
setFailedAndThrow(new TimeoutIOException(
"Timed out waiting for response after " + readTimeout));
if (elapsedNanos >= readTimeoutNanos && !future.isDone()) {
final TimeoutIOException e = new TimeoutIOException(
this + ": Failed to poll a response, timed out " + readTimeout);
if (setFailed(e)) {
throw e;
}
return null;
}
}
}

private ByteBuffer read(int length, boolean preRead) throws IOException {
private ReadBuffer read(int length, boolean preRead) throws IOException {
checkError();
if (future.isDone()) {
// Don't return null while items remain in the queue. onNext() may have delivered items just before
// onCompleted() fired.
return responseQueue.isEmpty() ? null : readFromQueue();
if (responseQueue.isEmpty()) {
return null;
}
} else {
// send gRPC onNext(..)
readBlock(length, preRead);
}

readBlock(length, preRead);

// poll buffer from queue
while (true) {
final ByteBuffer buf = readFromQueue();
if (buf == null) {
return null; // Stream ended
final ReadBlockResponseProto proto = poll();

@sodonnel sodonnel Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude suggests there is a problem if we have pre-read to the end of the stream so the stream is done, and then seek backwards. This is what it says, although I am not sure how the stream can be done and we are still reading, but it sounds plausible:

"Infinite spin in the read() while-loop when a backward seek leaves stale ahead-of-position responses in the queue and then the stream ends."

Client at position 50 MB seeks backward to 10 MB; reuseReadBuffer returns null; streamingReader is alive with ~30 stale responses (for offsets 51–80 MB) queued. After readBlock() sends a new request at 10
MB, poll() returns each stale response in turn. For each, getByteBuffer(staleProto, 10 MB) finds blockOffset(10 MB) < protoOffset(51+ MB) and returns null; read is null; hasRemaining is false; the loop continues. If the future
completes (stream done) before new responses arrive — e.g. server finishes sending to the old read boundary — poll() returns null at line 474-475. Line 511 NPEs. The loop has no exit path for a null proto, so even without the NPE
it would spin forever: there is no break or return null when proto is null and the stream is done."

@szetszwo szetszwo Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bug -- it should return null when the proto returned by poll() is null.

if (proto == null) {
return null;
}
if (buf.hasRemaining()) {
return buf;
final ByteBuffer buffer = getByteBuffer(proto, getPos());
Comment thread
sodonnel marked this conversation as resolved.
final ReadBuffer read = buffer != null ? new ReadBuffer(proto, buffer) : null;
if (hasRemaining(read)) {
LOG.debug("{}: read(length={}, preRead={}) returns {}", name, length, preRead, read);
return read;
}
// buf is empty: the server aligned its response to a checksum boundary
// before our current position and all bytes were skipped. Fetch the next
// response, which should start at or after our position.
}
}

ByteBuffer readFromQueue() throws IOException {
final ReadBlockResponseProto readBlock = poll();
if (readBlock == null) {
return null; // Stream ended
}
// The server always returns data starting from the last checksum boundary. Therefore if the reader position is
// ahead of the position we received from the server, we need to adjust the buffer position accordingly.
final ByteString data = readBlock.getData();
final ByteBuffer dataBuffer = data.asReadOnlyByteBuffer();
final long blockOffset = readBlock.getOffset();
final long pos = getPos();
if (pos < blockOffset) {
// This should not happen, and if it does, we have a bug.
setFailedAndThrow(new IllegalStateException(
this + ": out of order, position " + pos + " < block offset " + blockOffset));
}
final long offset = pos - blockOffset;
if (offset > 0) {
dataBuffer.position(Math.toIntExact(Math.min(offset, dataBuffer.limit())));
}
LOG.debug("{}: return response positon {}, length {} (block offset {}, length {})",
name, pos, dataBuffer.remaining(), blockOffset, data.size());
return dataBuffer;
}

private void releaseResources() {
Expand Down Expand Up @@ -575,12 +621,6 @@ StreamingReadResponse getResponse() {
return response.get();
}

private <T extends Throwable> void setFailedAndThrow(T throwable) throws T {
if (setFailed(throwable)) {
throw throwable;
}
}

private boolean setFailed(Throwable throwable) {
final boolean completed = future.completeExceptionally(throwable);
if (!completed) {
Expand Down Expand Up @@ -608,9 +648,9 @@ private void setCompleted() {
}

private void offerToQueue(ReadBlockResponseProto item) {
if (LOG.isDebugEnabled()) {
if (LOG.isTraceEnabled()) {
final ContainerProtos.ChecksumData checksumData = item.getChecksumData();
LOG.debug("{}: enqueue response offset {}, length {}, numChecksums {}, bytesPerChecksum={}",
LOG.trace("{}: enqueue response offset {}, length {}, numChecksums {}, bytesPerChecksum={}",
name, item.getOffset(), item.getData().size(),
checksumData.getChecksumsList().size(), checksumData.getBytesPerChecksum());
}
Expand Down
Loading
Loading