Gargi-jais11 commented on code in PR #9628:
URL: https://github.com/apache/ozone/pull/9628#discussion_r2762558436
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##########
@@ -86,22 +90,31 @@ public synchronized boolean read(ByteBuffer buffer) throws
IOException {
* In case of exception, this method catches the exception, logs a warning
message,
* and then continue closing the remaining resources.
*/
+ @Override
public synchronized void close() {
- if (blockFile == null) {
+ final File fileToClose = blockFile;
+ if (fileToClose == null) {
return;
}
Review Comment:
Check for **null** before copying `blockFile`.
```suggestion
if (blockFile == null) {
return; // Already closed
}
// Store file reference for logging before clearing
final File fileToClose = blockFile;
blockFile = null;
}
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##########
@@ -47,9 +48,12 @@ public synchronized boolean isOpen() {
/** Open the given file in read-only mode. */
public synchronized void open(File file) throws FileNotFoundException {
Preconditions.assertNull(blockFile, "blockFile");
- blockFile = Objects.requireNonNull(file, "blockFile == null");
- raf = new RandomAccessFile(blockFile, "r");
- channel = raf.getChannel();
+ final File f = Objects.requireNonNull(file, "blockFile == null");
+ final RandomAccessFile newRaf = new RandomAccessFile(f, "r");
+ final FileChannel newChannel = newRaf.getChannel();
+ blockFile = f;
+ raf = newRaf;
+ channel = newChannel;
}
Review Comment:
I think we should consider wrapping open() in try-catch to clean up on
failure .
I am saying this as I suggest that :
If `FileNotFoundException` is thrown: blockFile is set, but `raf` and
`channel `remain null.
Here the problem is that ` isOpen()` returns true (because blockFile !=
null), but the object isn't usable. And will the subsequent calls to `read()`
or `position()` will fail with NPE when accessing channel
--
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]
