HDDS-14370. RandomAccessFileChannel to implement Closeable#9905
HDDS-14370. RandomAccessFileChannel to implement Closeable#9905rich7420 wants to merge 9 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
| // 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?
| @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?
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