HDDS-16426. Fix thread-safety of positioned reads in OzoneCryptoInputStream

[taklwu]

What changes were proposed in this pull request?

Make sure all positioned reads in OzoneCryptoInputStream are thread-safe

Please describe your PR in detail:
Split from HDDS-15424 #11102 , this change only make sure OzoneCryptoInputStream has complied with thread-safe positional reads

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16426

How was this patch tested?

unit tests and compared with HDDS-16400 #11245

[Copilot]

🟡 Changes recommended

Unresolved thread-safety and invalid-position handling issues remain, with gaps in concurrency and failure-path test coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request improves thread safety for positioned reads in OzoneCryptoInputStream and adds related test coverage.

Changes:

• Synchronizes sequential and positioned read operations.
• Adds positioned ByteBuffer and byte-array read handling.
• Adds boundary, EOF, failure-recovery, and concurrency tests.

File summaries

File	Summary
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestOzoneCryptoInputStream.java	Adds functional and concurrency coverage.
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java	Adds synchronized positioned-read implementations.

Review details

Suppressed comments (1)

hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestOzoneCryptoInputStream.java:255

• The read-only check in read(long, ByteBuffer) runs before seek and before adjustReadPosition can set either adjustment field, so this non-boundary position never exercises the cleanup path described by the test. It only verifies early rejection and cannot catch stale adjustment state after a later read failure; add a failure injected after adjustment or make that claim a separate test.

      // Seek to a non-boundary offset so adjustReadPosition sets readPositionAdjustedBy.
      s.seek(100);
      assertThrows(ReadOnlyBufferException.class, () -> s.read(100, readOnly));

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Inherited cursor-sensitive APIs remain unsynchronized and can race with positioned reads.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java:169

• getPos() is protected from exposing the temporary positioned-read cursor, but inherited CryptoInputStream.available() is not. That method derives its result from the wrapped stream's current position and outBuffer, so a concurrent call can observe the temporary seek position instead of the sequential cursor. Override available() on the same monitor as the other cursor-sensitive methods.

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[jojochuang]

LGTM. Good fix serializing cursor-moving APIs with positioned seek–read–restore, and the concurrency tests (including skip vs positioned read) match the failure mode.

One follow-up worth considering: override available() with the same monitor as getPos() / positioned reads — inherited CryptoInputStream.available() can reflect the temporary seek during a positioned read if something calls it concurrently.

[taklwu]

One follow-up worth considering: override available() with the same monitor as getPos() / positioned reads — inherited CryptoInputStream.available() can reflect the temporary seek during a positioned read if something calls it concurrently.

added synchronized to available()

[jojochuang]

Merged. Thanks @taklwu !!