ppkarwasz commented on code in PR #754:
URL: https://github.com/apache/commons-compress/pull/754#discussion_r2596376658
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -191,13 +191,16 @@ private TarFile(final Builder builder) throws IOException
{
while ((entry = getNextTarEntry()) != null) {
entries.add(entry);
}
- } catch (final IOException ex) {
+ } catch (final IOException | IllegalArgumentException e) {
Review Comment:
I’m wondering whether extending the catch clause from `IOException` to
`IOException | IllegalArgumentException` is the right boundary. If the
intention is to shield users from **any** unexpected internal failures we
should rather catch `Exception`.
However, I see unchecked exceptions here as symptoms of underlying bugs.
Masking them in the constructor can improve user experience, but only hides the
bugs, instead of solving them.
In the specific case of COMPRESS-714, the `IllegalArgumentException` comes
from a bug in `SeekableInMemoryByteChannel`. According to the `position()`
contract, providing a position larger than the channel size should not result
in an exception, so the current implementation violates that contract:
https://github.com/apache/commons-compress/blob/b83da24c57ec32cf4c4ee7dcb58b09861c9c17ce/src/main/java/org/apache/commons/compress/utils/SeekableInMemoryByteChannel.java#L114-L121
--
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]
