Fix unhandled UnsupportedOperationException in Fallocate #1010
Conversation
| } | ||
|
|
||
| public Fallocate fromOffset(long offset) { | ||
| public Fallocate fromOffset(long offset) throws IllegalArgumentException { |
There was a problem hiding this comment.
It's very uncommon to list unchecked exceptions in the throws clause. The compiler does not benefit from it in any way.
Let's not introduce this as a new pattern.
There was a problem hiding this comment.
This unhandled UnsupportedOperationException bug could probably be prevented if the code author sees the throws clause in the IDE, so I would like to have it when the list is not too long.
There was a problem hiding this comment.
If that's the goal, please add Javadoc. Java throws is for the compiler, Javadoc @throws is for the developer. As an added benefit, it allows you to describe in which situations the exception is thrown!
| try { | ||
| return new Fallocate(channel, getDescriptor(channel), final_filesize); | ||
| } catch (final UnsupportedOperationException exception) { | ||
| // File descriptor is not supported: fd is null and unavailable |
There was a problem hiding this comment.
This solves the problem in the wrong place. The entire try/catch dance can be avoided by not throwing UnsupportedOperationException in getDescriptor to start with. Just have it return 0 (like you are simulating here) or -1 (as is done in UdpSocketHandler).
| errno = result == 0 ? 0 : Native.getLastError(); | ||
| } else if (IS_POSIX) { | ||
| errno = FallocateHolderPOSIX.posix_fallocate(fd, offset, final_filesize-offset); | ||
| if (fd != 0) { |
There was a problem hiding this comment.
It'd be more robust to check for fd > 2 instead of fd != 0, as we can be quite sure the fd will not be stdout or stderr either.
There was a problem hiding this comment.
We just need to remember that the UnsupportedOperationException was thrown. And fd > 2 does not mean anything neither stdout nor stderr, what about negative numbers? That seems unnecessary.
There was a problem hiding this comment.
On POSIX systems, file descriptors are non-negative numbers. The file descriptors 0, 1 and 2 represent stdin, stdout and stderr respectively. See https://en.wikipedia.org/wiki/File_descriptor for an explanation.
(And non-POSIX systems we don't care about here, as fallocate is not supported on them anyways).
There was a problem hiding this comment.
Yeah, if there are negative numbers or 0, 1 and 2, then it must be non-POSIX systems, so legacyFill must be used.
| isUnsupported = true; | ||
| // fd is null and unavailable | ||
| if (Platform.isWindows()) { | ||
| // Windows do not create sparse files by default, so just write a byte at the end. |
There was a problem hiding this comment.
The point of fallocate is to efficiently pre-allocate disk space for sparse files (allocate the blocks, but don't write the zeroes).
If Windows does not create sparse files anyways, then why do we need this special case? We already deal with the unsupported case through legacyFill below.
There was a problem hiding this comment.
legacyFill fills the file with random data. It's wasting SSD life time because when the useful data get in, the random data have to be erased. However it does not need to erase when writing zeros. So I suggest legacyFill should fill the file with zeros. And for Windows, this is better, because there's only one API call and does not use a large buffer, so it's faster (suggested by https://stackoverflow.com/questions/18031841/pre-allocating-drive-space-for-file-storage referenced in this file).
There was a problem hiding this comment.
Depending on filesystem (and most notably full-disk encryption) the SSD erase argument would indeed be a valid one.
As far as I remember, the "fill with random data" approach was to prevent transparent disk compression from getting in the way of allocating sufficient space. But it may have become less relevant over time with ever growing disk capacity.
If we keep this special case for Windows, can you it to where the unsupported case is handled? (probably where legacyFill() is called) Then it is clear that this is not an equivalent of fallocate, but rather an optimized alternative for when fallocate is unsupported.
Sorry, there were some issues that GitHub does not let me to update the branch with changes to GitHub actions, so I recreated #1008. Original indentation style is restored. fd with the value 0 is never actually used. I'm not sure what is unsafe to write a byte at the end of a file on Windows. The data written is not going to be used other than to allocate disk space, and filling the file with random data instead of zeros can shorten SSD life time. I have run this code on Windows.