ppkarwasz commented on PR #801:
URL: https://github.com/apache/commons-io/pull/801#issuecomment-3413177310
I believe this PR **does** address a key part of IO-802: namely, it prevents
shared buffer mutation between `skipFully()` (or `skip`) and concurrent reads
of decompressing streams, which was the root of the multithreading failure
observed.
I’m not entirely sure how `InflaterInputStream` itself uses the buffer, but
its subclasses like `GZIPInputStream` and `ZipInputStream` definitely update a
CRC as they read data. If another thread were to perform a skip operation on a
different `InputStream` instance while sharing the same buffer, it could
corrupt the data being validated and cause CRC verification to fail.
However, I don’t think this PR completely closes every corner case. In
particular:
1. **Reentrancy / nested skip calls**
Some streams (for example from Commons Compress, such as
`GzipCompressorInputStream`) may internally call `IOUtils.skip(...)` or
`skipFully(...)` from within their own `read(...)` implementations.
Even with a `ThreadLocal` that internal skip uses the *same* buffer as
an external skip.
2. **Thread-local / buffer ownership tracking**
To guard against those cases, we should adopt a pattern similar to
Log4j’s: maintain a flag to detect whether the local buffer is in use.
I’m happy to adapt this PR to follow that pattern.
--
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]
