HDDS-14370. RandomAccessFileChannel to implement Closeable#9905
HDDS-14370. RandomAccessFileChannel to implement Closeable#9905rich7420 wants to merge 10 commits intoapache:masterfrom
Conversation
|
@Gargi-jais11 please take a look |
Gargi-jais11
left a comment
Gargi-jais11
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for updating the patch. LGTM!
|
Thanks so much @adoroszlai , @yandrey321 and @Gargi-jais11 |
| * {@link FileChannel#close()} is final (inherited), so we implement a minimal {@link FileChannel} | ||
| * and throw from {@link #implCloseChannel()} to simulate close failure. | ||
| */ | ||
| private static final class FailingCloseFileChannel extends FileChannel { |
There was a problem hiding this comment.
could we find other to verify the close behaviour? there are so much code that are unused
There was a problem hiding this comment.
Good catch! I tried to replace FailingCloseFileChannel and TrackingRandomAccessFile with Mockito mock to eliminate the boilerplate.
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @rich7420 left few comments
| assertDoesNotThrow(c::close); | ||
| verify(spyRaf).close(); |
There was a problem hiding this comment.
| assertDoesNotThrow(c::close); | |
| verify(spyRaf).close(); | |
| assertDoesNotThrow(c::close); | |
| verify(failingChannel).close(); | |
| verify(spyRaf).close(); |
Optional: We can add verify(failingChannel).close(); to explicitly verify the channel close was attempted.
There was a problem hiding this comment.
nit : Not related to your changes, but there is a typo in javadoc - ture should be true
| @Test | ||
| void readWithZeroSizedBuffer() throws Exception { | ||
| final RandomAccessFileChannel c = new RandomAccessFileChannel(); | ||
| final File f = tempDir.resolve("test-file").toFile(); | ||
| try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) { | ||
| raf.write(new byte[]{1, 2, 3, 4, 5}); | ||
| } | ||
|
|
||
| c.open(f); |
There was a problem hiding this comment.
we should also have c.close()
|
@sreejasahithi thanks for the review! |
|
@peterxcli would you like to take another look? |
| final File f = Objects.requireNonNull(file, "blockFile == null"); | ||
| final RandomAccessFile newRaf = new RandomAccessFile(f, "r"); | ||
| try { | ||
| final FileChannel newChannel = newRaf.getChannel(); |
There was a problem hiding this comment.
I think this wont throw, so we can remove the exception handling block
There was a problem hiding this comment.
Done. Removed the try-catch block since getChannel() does not throw checked exceptions.
| // Closeable contract: close via try-with-resources helper | ||
| closeAndVerify(c); | ||
| assertFalse(c.isOpen(), "should be closed after try-with-resources"); | ||
| assertDoesNotThrow(c::close, "double close should be safe"); |
There was a problem hiding this comment.
if double close happened, then the programming logic must be wrong, right?
There was a problem hiding this comment.
I think this test ensures we strictly adhere to the contract, regardless of the caller's application logic.
| @Override | ||
| public synchronized void close() { | ||
| if (blockFile == null) { | ||
| final File fileToClose = blockFile; |
There was a problem hiding this comment.
why add a new variable? doesn't the original logic work?
There was a problem hiding this comment.
Oh I think it fixes an existing logging bug. In the original code, blockFile was set to null before closing the channel, so if an IOException occurred, the log incorrectly printed null instead of the file name.
6 participants
What changes were proposed in this pull request?
The class already had a close() method but didn't implement Closeable, which could lead to resource leaks if developers miss calling close().
What is the link to the Apache JIRA
HDDS-14370
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/22942351747
reopen
#9628