8224617: (fs) java/nio/file/FileStore/Basic.java found filesystem twice

[Brian Burkhalter]

> On May 26, 2019, at 1:58 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> 
> On 25/05/2019 00:39, Brian Burkhalter wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8224617 <https://bugs.openjdk.java.net/browse/JDK-8224617>
>> http://cr.openjdk.java.net/~bpb/8224617/webrev.00/ <http://cr.openjdk.java.net/~bpb/8224617/webrev.00/>
>> 
>> The proposed change adds a method “checkMountPoints()” to the FileUtils test library class and in FileStore/Basic.java replaces the use of FileUtils.areFileSystemsAccessible() with the new method. The new method accomplishes the same thing as areFileSystemsAccessible() but additionally checks that there are not duplicate mount points on the system. This latter would indicate a file system misconfiguration which would formerly have appeared as a FileStore failure but with the present change is flagged explicitly as a system configuration problem.
> The approach looks okay although 90s is a long time for a test to wait for `df` to complete.

This is the value that is used in the pre-existing method FileUtils.areFileSystemsAccessible() which was intended mainly to detect whether ‘df’ hangs. The Process.waitFor() method appears actually to return after about only 0.05 ms at least on my system.

> Also I'm not sure that checkMountPoints is the best name for a method that returns a boolean to indicate if the file systems are usable for testing. Something like areAllMountPointsAccessible() might be more obvious when reading the test.

Yes I changed the name more than once while writing this:

--- a/test/jdk/java/nio/file/FileStore/Basic.java
+++ b/test/jdk/java/nio/file/FileStore/Basic.java
@@ -115,7 +115,7 @@
         /**
          * Test: Enumerate all FileStores
          */
-        if (FileUtils.checkMountPoints()) {
+        if (FileUtils.areAllMountPointsAccessible()) {
             FileStore prev = null;
             for (FileStore store: FileSystems.getDefault().getFileStores()) {
                 System.out.format("%s (name=%s type=%s)\n", store, store.name(),

--- a/test/lib/jdk/test/lib/util/FileUtils.java
+++ b/test/lib/jdk/test/lib/util/FileUtils.java
@@ -256,7 +256,7 @@
      * @throws RuntimeException if there are duplicate mount points or some
      * other execution problem occurs
      */
-    public static boolean checkMountPoints() {
+    public static boolean areAllMountPointsAccessible() {
         final AtomicBoolean areMountPointsOK = new AtomicBoolean(true);
         if (!IS_WINDOWS) {
             Thread thr = new Thread(() -> {


>> One question about the new code is whether the test at lines 269-279 of the new version of FileUtils should not assume that duplicates occur in succession in the output of ‘df.’ (This is assumed in Basic in the testing for dulicate FileStores). An alternative would be to accumulate each new non-null entry into a Set and check whether adding the entry actually changed the Set. If it did not then there is a duplicate.
>> 
> Are there any scenarios where FileSystem::getFileStores should ignore the duplicates?

It is not specified whether it does or does not as far as I can see. The test FileStore/Basic historically only flags duplicates if they are successors in the FileStore iteration. The order of the elements in the iteration is undefined according to the specification however. I would think that either duplicates should be allowed in the iteration or if they are not allowed then the error should be independent of their order. In either of these cases the test has been wrong and still is.

Brian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190528/553d2006/attachment-0001.html>