You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/01/27 14:42:58 UTC
[GitHub] [commons-io] boris-unckel opened a new pull request #192: [IO-707] Add optional postcondition checks with assert
boris-unckel opened a new pull request #192:
URL: https://github.com/apache/commons-io/pull/192
Fixes [https://issues.apache.org/jira/browse/IO-707](https://issues.apache.org/jira/browse/IO-707)
----------------------------------------------------------------
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
[GitHub] [commons-io] coveralls commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768642694
[](https://coveralls.io/builds/36650052)
Coverage decreased (-0.1%) to 89.092% when pulling **a559f8a99d97a996b9ecba48680ed04588205ce6 on boris-unckel:assert_proposal** into **5f73593d999feccc93f6e8190af22d52726bba14 on apache:master**.
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768802353
@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.
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").
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel edited a comment on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel edited a comment on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768802353
@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").
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel commented on a change in pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #192:
URL: https://github.com/apache/commons-io/pull/192#discussion_r565826684
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1168,7 +1169,9 @@ static String decodeUrl(final String url) {
*/
public static File delete(final File file) throws IOException {
Objects.requireNonNull(file, "file");
- Files.delete(file.toPath());
+ Path path = file.toPath();
+ Files.delete(path);
+ assert Files.notExists(path, PathUtils.NOFOLLOW_LINK_OPTION_ARRAY) : "File still exists " + path;
Review comment:
@kinow Please find my feedback below. It addresses tests (not production) for a file system constellation which can be reproduced with extended attributes.
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel commented on a change in pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #192:
URL: https://github.com/apache/commons-io/pull/192#discussion_r565820479
##########
File path: pom.xml
##########
@@ -394,6 +394,7 @@ file comparators, endian transformation classes, and much more.
</classpathDependencyExcludes>
<forkCount>1</forkCount>
<reuseForks>false</reuseForks>
+ <enableAssertions>true</enableAssertions>
Review comment:
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.
----------------------------------------------------------------
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
[GitHub] [commons-io] garydgregory commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768449132
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...
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel closed pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #192:
URL: https://github.com/apache/commons-io/pull/192
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768409327
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...
----------------------------------------------------------------
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
[GitHub] [commons-io] garydgregory commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-925899550
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?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel edited a comment on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel edited a comment on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768802353
@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").
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel edited a comment on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel edited a comment on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-768802353
@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.
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").
----------------------------------------------------------------
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
[GitHub] [commons-io] boris-unckel commented on pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on pull request #192:
URL: https://github.com/apache/commons-io/pull/192#issuecomment-925953082
[https://issues.apache.org/jira/browse/IO-725](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.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a change in pull request #192: [IO-707] Add optional postcondition checks with assert
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #192:
URL: https://github.com/apache/commons-io/pull/192#discussion_r565660147
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1168,7 +1169,9 @@ static String decodeUrl(final String url) {
*/
public static File delete(final File file) throws IOException {
Objects.requireNonNull(file, "file");
- Files.delete(file.toPath());
+ Path path = file.toPath();
+ Files.delete(path);
+ assert Files.notExists(path, PathUtils.NOFOLLOW_LINK_OPTION_ARRAY) : "File still exists " + path;
Review comment:
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).
##########
File path: pom.xml
##########
@@ -394,6 +394,7 @@ file comparators, endian transformation classes, and much more.
</classpathDependencyExcludes>
<forkCount>1</forkCount>
<reuseForks>false</reuseForks>
+ <enableAssertions>true</enableAssertions>
Review comment:
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?
----------------------------------------------------------------
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