[IO-707] Add optional postcondition checks with assert

[boris-unckel]

Fixes https://issues.apache.org/jira/browse/IO-707

[garydgregory]

Hi @boris-unckel
Hm, we do not use these types of asserts anywhere, needs discussion on the mailing list, please see the thread I am about to start there...

[garydgregory]

Hm... it seems to me that any file operation in an app (through Commons IO or otherwise) is always at risk of being "out-of-sync" or simply subverted by another process or user doing some operations on disk, so this is going to be confusing at best. For example, let's say I perform some Commons IO file operation, then the JVM or GC pauses my thread for a "long" time, during which something happens on disk which violates my app's expectations once it wakes up... now what?
If the pause happened before the assert, the assert catches something... if it happens after, I know nothing and my app calling Commons IO still needs to handle the situation, assert or no assert. I'm just worried that we are cherry picking a few places to add asserts that might not be useful and will cause users to ask why asserts are here but not there, and so on... looking f/w to thoughts from the community...

[kinow]

I haven't used ­or read about assertions in Java in a while. Unless something changed in recent releases, I think this enables the -ea flag for build time. But users of the API would still have it disabled by default in OpenJDK/Oracle JVM's.

So the new code with assertions wouldn't be used, and I think using assertions in production code (not in tests) used to be discouraged?

[boris-unckel]

Thanks for taking time to review this. Yes, assertions are not intended to be used in production. Especially AssertionErrors (assert does throw an error, not an exception) should not be used for application logic.

[kinow]

I'm not sure if that's really necessary to have in this method? From the docs for this file, it used to be a useful alternative for File#delete that returned a boolean. But now with the JVM (from 8 I think?) Files#delete they both have similar behaviour.

So if the file cannot be deleted, an exception is thrown. If the JVM implementation (or underlying OS libraries, or maybe a NFS or virtual file system) reported the file as deleted, but the file still exists, then both Files#delete and this method will behave the same way.

I'm not sure if having an assertion error here would be helpful to users of these platforms. They'd probably have to rely on the JVM, or have their code to check for the file still existing (they could be using the JVM Files#delete somewhere else in the code).

[boris-unckel]

@kinow Please find my feedback below. It addresses tests (not production) for a file system constellation which can be reproduced with extended attributes.

[coveralls]

Coverage decreased (-0.1%) to 89.092% when pulling a559f8a on boris-unckel:assert_proposal into 5f73593 on apache:master.

[boris-unckel]

@garydgregory Thank you for your time to review and place it for public review.

If the pause happened before the assert, the assert catches something... if it happens after, I know nothing and my app calling Commons IO still needs to handle the situation, assert or no assert.

The use case you're describing is not improved by the assert or just slightly. I'm thinking more about cases where the underlying file system in combination with the JRE does not behave as expected:
The following is a plain vanilla Fedora 33 with ext4 filesystem. Unfortunately in German :-(

For /tmp it says "IOCTL not matching to read flags of /tmp".

sudo lsattr /    
[sudo] Passwort für borisunckel:    
-----------I--e----- /etc    
lsattr: Unpassender IOCTL (I/O-Control) für das Gerät Beim Lesen der Flags von /run    
lsattr: Die Operation wird nicht unterstützt Beim Lesen der Flags von /lib64    
--------------e----- /lost+found    
lsattr: Unpassender IOCTL (I/O-Control) für das Gerät Beim Lesen der Flags von /sys    
--------------e----- /usr    
lsattr: Unpassender IOCTL (I/O-Control) für das Gerät Beim Lesen der Flags von /proc    
--------------e----- /mnt    
lsattr: Die Operation wird nicht unterstützt Beim Lesen der Flags von /sbin    
--------------e----- /srv    
lsattr: Die Operation wird nicht unterstützt Beim Lesen der Flags von /lib    
lsattr: Unpassender IOCTL (I/O-Control) für das Gerät Beim Lesen der Flags von /tmp    
lsattr: Die Operation wird nicht unterstützt Beim Lesen der Flags von /bin    
--------------e----- /root    
--------------e----- /media    
--------------e----- /var    
--------------e----- /home    
lsattr: Unpassender IOCTL (I/O-Control) für das Gerät Beim Lesen der Flags von /dev    
--------------e----- /boot    
--------------e----- /opt    

Working with commons-io 2.5 I had no trouble to build WildFly-Core, with 2.8.0 it fails. The first fixes I already provided (former PRs not this one), but it is not over. A workaround is to set java.io.temp to a directory with "--------------e-----" attributes.

The asserts help to find the original place of fail and not the top level one (like "delete fails due to directory is not empty").

[garydgregory]

Hi @boris-unckel and All:
A lot of time has passed since we last iterated on this issue...
May you please rebase on master if this is still relevant with master?

[boris-unckel]

https://issues.apache.org/jira/browse/IO-725 is related to this. As long as only Ubuntu (with a different file system) is accepted as prove for bugs, this here is not useful.