[GitHub] [commons-vfs] MaxKellermann commented on pull request #154: Rework SoftRefFilesCache locking

[GitBox <gi...@apache.org>]

MaxKellermann commented on pull request #154:
URL: https://github.com/apache/commons-vfs/pull/154#issuecomment-756130584


   > You mention that there are bugs, ok. But what are they? At the very least we need a list so users can know if a new release addresses an issue they may have.
   
   Each commit which addresses a bug describes it in the commit message.
   
   In particular, those 4 commits address real bugs, each documented:
   https://github.com/apache/commons-vfs/pull/154/commits/98d646e95eed47b11bdb063911161b667223df51
   https://github.com/apache/commons-vfs/pull/154/commits/f210ac6f2a338f2aeb18217b3c408c1dd92eacca
   https://github.com/apache/commons-vfs/pull/154/commits/eae0cd551978a83ffaeffa77622a8dbc8762acb0
   https://github.com/apache/commons-vfs/pull/154/commits/b946d8077a686fc2af236b81a56f55db74e727c0
   
   The rest is just refactoring and optimization.
   
   > Without unit tests that fail without the main changes, the code may just be changed and regress to the old behavior in the future, only to be fixed again later, round and round.
   
   How do you unit-test race conditions / TOCTOU bugs?
   
   I had a look at the current unit tests for this class - and despite it being allegedly thread-safe, there is not a single unit test testing anything about thread-safety. Hey, the unit tests are so short that I can paste *all of it* here:
   https://github.com/apache/commons-vfs/blob/3916dd2a2f88c9ae27d040acc11c63eeb7b1ca64/commons-vfs2/src/test/java/org/apache/commons/vfs2/cache/SoftRefFilesCacheTests.java#L31-L35
   That's all!
   
   And this is a class whose `SoftRefReleaseThread` was broken for 5 years (due to an inverted check added by the completely-bad-commit 9e360ca017b1f639957c799a8b7b64047c286d93) that the code could *never* run, leading to an unbounded memory leak. This was fixed by commit https://github.com/apache/commons-vfs/commit/f09035290b1cdcf54f40c331e8c84d925eb454b7 which introduced a new bug (fixed by this PR), but without any unit tests (and with a very confusing commit message which missed the point).
   
   If there was an example in this library's unit tests on how to write a unit test for such a bug, I could give it a try, but I see none. This would be the first time such a unit test for a race condition fix is required.
   
   But yes, basically we agree that having a unit test would be great, but my motivation to figure this out from scratch doesn't exist.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org