8272964: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted

[bplb]

In the interrupt case, if the copy does not throw an IOException, rather than using the copy duration versus a threshold as the criterion for failure, instead check whether the target file does not exist, and if it does not, then make the test fail as this would indicate that the copy was in fact interrupted but did not throw an exception in response.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8272964: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted

Reviewers

• Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5260/head:pull/5260
$ git checkout pull/5260

Update a local copy of the PR:
$ git checkout pull/5260
$ git pull https://git.openjdk.java.net/jdk pull/5260/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5260

View PR using the GUI difftool:
$ git pr show -t 5260

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5260.diff

[bridgekeeper]

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[bplb]

Before this PR was created, this test had been run 700 times on each of four different platforms without any failures.

[openjdk]

@bplb The following label will be automatically applied to this pull request:

• nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (ebaa7b86)

[msheppar]

That's look like a good approach, because the duration threshold test is always subject to the load and the number of extant threads in the test system ... for recent failures the number of threads is significant (> 1000)
Also, when following the copy calls flows there is significant amount of processing prior to the actual copy activity taking place and that occurs in Cancellable in a newly launched "copying thread". What happens if the copy thread has yet to run?

Additionally, would it be useful for the copy interrupter thread to check for the existence of the target file, prior to invoking the interrupt

e.g.
while (Files.notExist(target))
Thread.sleep(...);

[bplb]

My testing showed that if the interrupt is set on the main thread, the copy thread, before the copy actually starts, then the interrupt is missed and the copy runs to completion.

As for the Files.notExists(target) loop, I tried that in the previous attempt to fix this issue and it appeared that on Windows this call is too slow to use in this context and does not fix the problem.

[dfuch]

I like that!

[openjdk]

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272964: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 13 new commits pushed to the master branch:

• 9166ba3: 8272973: Incorrect compile command used by TestIllegalArrayCopyBeforeInfiniteLoop
• 49b2789: 8262751: RenderPipelineState assertion error in J2DDemo
• a3308af: 8272836: Limit run time for java/lang/invoke/LFCaching tests
• c4c76e2: 8272811: Document the effects of building with _GNU_SOURCE in os_posix.hpp
• 673ce7e: 8272873: C2: Inlining should not depend on absolute call site counts
• 7212561: 8267188: gc/stringdedup/TestStringDeduplicationInterned.java fails with Shenandoah
• e36cbd8: 8242847: G1 should not clear mark bitmaps with no marks
• 2ef6871: 8272447: Remove 'native' ranked Mutex
• 63e062f: 8236176: Parallel GC SplitInfo comment should be updated for shadow regions
• c5a2712: 8272850: Drop zapping values in the Zap* option descriptions
• ... and 3 more: https://git.openjdk.java.net/jdk/compare/0e7288ffbf635b9fdb17e8017e9a4f673ca0501d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[msheppar]

the solution looks hunky dory, but one question on the call flow in Unix for Files.copy: For the interruptible scenario, an auxiliary thread is used by Cancellable to process the copy. If the "main" thread associated with the Files.copy has yet to reach a call on the actual copy thread t.join when it is interrupted or the interrupt is delivered, then it looks like the interrupt will be missed and the InterruptExcpetion won't be generated. So is there a case for checking the interrupt status of the current thread, in the copy call flow, prior to invoking Cancellable.runInterruptibly, or before t.start for executing the actual copy or before t.join in the runInterruptibly method ? as such increasing the possibility of intercepting the Thread::interrupt in the test?
So in the call flow if the current thread's interrupt status equals interrupted prior to invoking Cancellable.runInterruptibly then throw an IOException at that point, and additionally test the interrupt status prior to invoking t.start and t.join in runInterruptibly.
The point I'm attempting to make is that the t.join in runInterruptibly is the juncture at which an interrupt can be caught, and prior to that, if I understand Thread::interrupt correctly, only the interrupt status will be set.

As such, is the test failure not also alluding a potential flaw in the interruptibility of the File.copy? That there is the possibility of an interrupt being delivered prior to the actual invocation of the "real copy" ?

[bplb]

I tested three scenarios:

1. interrupt the thread which calls Files.copy() prior to calling Files.copy();
2. self-interrupt the thread which calls Files.copy() in Cancellable on the line before t.join().
3. self-interrupt the thread which calls Files.copy() in Cancellable on the line after t.join().

In the first two cases an IOException triggered by an InterruptedException was thrown. Therefore I don't believe that there is any need to check isInterrupted() to avoid missing an interrupt made before spawning the "NIO-Task" thread in which the Cancellable task is executed.

The third case however reveals that if the thread which calls Files.copy() is interrupted in runInterruptibly() on the line after t.join(), then the copy succeeds and no InterruptedException is thrown. So there is a small window in time between t.join() and the return from Files.copy() when the interrupt wil be ignored.

[bplb]

/integrate

[openjdk]

Going to push as commit dfeb413.
Since your change was applied there have been 29 commits pushed to the master branch:

• a033aa5: 8273072: Avoid using += in configure
• b92214a: 8272480: Remove Mutex::access rank
• 596b075: 8258465: Headless build fails due to missing X11 headers on linux
• a49a0c5: 8273062: Generation::refs_discovery_is_xxx functions are unused
• ebd62bc: 8272846: Move some runtime/Metaspace/elastic/ tests out of tier1
• 46684a4: 8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out
• c925c7f: 8273020: LibraryCallKit::sharpen_unsafe_type does not handle narrow oop array
• 76baace: 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code
• b16a04e: 8271186: Add UL option to replace newline char
• d732c30: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
• ... and 19 more: https://git.openjdk.java.net/jdk/compare/0e7288ffbf635b9fdb17e8017e9a4f673ca0501d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@bplb Pushed as commit dfeb413.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from Alan Bateman on nio-dev:

On 26/08/2021 21:13, Brian Burkhalter wrote:

:
The third case however reveals that if the thread which calls `Files.copy()` is interrupted in `runInterruptibly()` on the line _after_ `t.join()`, then the copy succeeds and no `InterruptedException` is thrown. So there is a small window in time between `t.join()` and the return from `Files.copy()` when the interrupt wil be ignored.

I don't immediately see an issue there. A thread can be interrupted at
any time. If a thread in Files.copy is interrupted after the copy has
completed but before the method returns then it just means that
Files.copy will complete (successfully) but with the interrupt status set.

-Alan.