Fix #329: Empty RAF creation on read#330
Conversation
| handle.read(); // this will fail as there is no underlying file! | ||
| fail("Read successfully from non-existing file!"); | ||
| } | ||
| catch (final Exception exc) { |
There was a problem hiding this comment.
How about catching only IOException instead of Exception here?
There was a problem hiding this comment.
Good point, I just pushed an updated version that does just that.
gab1one
force-pushed
the
fix-raf-creation
branch
from
e868c41 to
ceb430f
Compare
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
This is awesome! Splitting the RAF logic along read/write lines is exactly the right change to finally fix this bugbear. I have only a few minor comments+nitpicks, and then we should be good to merge.
| assertFalse(nonExistentFile.exists()); | ||
|
|
||
| final FileLocation loc = new FileLocation(uri); | ||
| final DataHandle<?> handle = dhs.create(loc); |
There was a problem hiding this comment.
We should use try-with-resources here, no?
try (final DataHandle<?> handle = dhs.create(loc)) {
...
}
So that it definitely gets closed at the end, regardless of the outcome of the test
| @Override | ||
| public long length() throws IOException { | ||
| return exists() ? raf().length() : -1; | ||
| return exists() ? writeRaf().length() : -1; |
There was a problem hiding this comment.
Conceptually, this should be readRaf() since querying the length is a "read" operation. (I realize that in practice, it won't matter due to the exists() check beforehand, but the code looks strange/wrong with writeRaf() here.)
| */ | ||
| private RandomAccessFile readRaf() throws IOException { | ||
| if (raf == null) { | ||
| if (!exists()) { |
There was a problem hiding this comment.
I would prefer this exists() check live in the initRAF() method. It can be done by making the method initRAF(final boolean create) and then writing if (!create && !exists()) throw .... Then, here in readRAF(), simply put if (raf == null) initRAF(false);—and correspondingly, initRAF(true) in the writeRAF() method. Then the two methods will be symmetrical, and IMHO easier to understand.
Note also the case of RAF—it is an acronym, so should be all capitals, not camel case Raf. Maybe we want to call them reader() and writer() instead of readRaf() and writeRaf()? I feel like readRaf() and writeRaf() are somehow too long/unwieldy.
There was a problem hiding this comment.
Great suggestion, this makes the code much cleaner :)
gab1one
force-pushed
the
fix-raf-creation
branch
from
ceb430f to
83fd2a1
Compare
|
Thanks for the input @ctrueden I integrated your comments and it should be good to go now. |
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
Just a handful of little things now!
| @Override | ||
| public long offset() throws IOException { | ||
| return raf().getFilePointer(); | ||
| return exists() ? writer().getFilePointer() : 0; |
| /** Gets the random access file object backing this FileHandle. */ | ||
| public RandomAccessFile getRandomAccessFile() throws IOException { | ||
| return raf(); | ||
| return writer(); |
There was a problem hiding this comment.
We should amend the javadoc of this method to indicate that calling this method will create the file if it does not already exist.
| * independently of the underlying file existing on disk. This allows us to | ||
| * create a new file for writing. | ||
| * | ||
| * @return the internal {@link RandomAccessFile} or creates if if needed. |
There was a problem hiding this comment.
Should be the internal {@link RandomAccessFile}, creating it if needed.
| * if the underlying file exists on disk. This prevents accidental creation of | ||
| * an empty RAF file when calling read operations on a non-existent file. | ||
| * | ||
| * @return the internal {@link RandomAccessFile} or creates if if needed. |
There was a problem hiding this comment.
Should be the internal {@link RandomAccessFile}, creating it if needed.
| } | ||
|
|
||
| /** | ||
| * Initializes the {@link RandomAccessFile}, |
| * @param create whether to create the {@link RandomAccessFile} if the | ||
| * underlying file does not exist yet. | ||
| * @throws IOException if the {@link RandomAccessFile} could not be created, | ||
| * or the backing file does not exists and the {@code create} |
| * underlying file does not exist yet. | ||
| * @throws IOException if the {@link RandomAccessFile} could not be created, | ||
| * or the backing file does not exists and the {@code create} | ||
| * parameter was set to {@code true}. |
When trying to read from a non-existing file, the FileHandle created a RandomAccessFile, which resulted in an empty file appearing on disk.
gab1one
force-pushed
the
fix-raf-creation
branch
from
83fd2a1 to
15b70a1
Compare
|
|
Thanks @gab1one !! |
3 participants
When trying to read from a non-existing file, the FileHandle creates a RandomAccessFile, which results in an empty file appearing on disk. This PR fixes this by throwing an IOException when trying to read from a non-existent file.