Skip to content

Comments

[NETBEANS-583] Avoid errors related to ClosedByInterruptException#1000

Closed
eirikbakke wants to merge 1 commit intoapache:masterfrom
eirikbakke:NETBEANS-583
Closed

[NETBEANS-583] Avoid errors related to ClosedByInterruptException#1000
eirikbakke wants to merge 1 commit intoapache:masterfrom
eirikbakke:NETBEANS-583

Conversation

@eirikbakke
Copy link
Contributor

@eirikbakke eirikbakke commented Oct 31, 2018

See the related discussion in #854 .

In commit 4b82e6a, a lot of calls to "new FileInputStream/FileOutputStream" were changed to NIO-based Files.newInputStream/newOutputStream. This caused a new class of intermittent bugs to appear, because the NIO-based streams, unlike FileInputStream/FileOutputStream, all throw ClosedByInterruptException if the current thread is interrupted. Few of the current call sites, nor the APIs they implement, are designed to allow this kind of fine-grained interrupt handling.

This commit carefully goes through each of the changes in 4b82e6a, redirecting potentially unsafe uses of Files.newInputStream/newOutputStream to a new class FilesNI, which currently just creates a regular old FileInputStream/FileOutputStream. An NIO-based approach, if necessary, can be implemented there at a later point if deemed worthwhile.

ClosedByInterruptException currently appears in bug reports NETBEANS-609, NETBEANS-1340, NETBEANS-1061, NETBEANS-920, NETBEANS-583, NETBEANS-1197, and NETBEANS-1357.

The JarClassLoader case was handled separately in NETBEANS-1197.

In commit 4b82e6a, a lot of calls to
"new FileInputStream/FileOutputStream" were changed to NIO-based
Files.newInputStream/newOutputStream. This caused a new class of intermittent
bugs to appear, because the NIO-based streams, unlike
FileInputStream/FileOutputStream, all throw ClosedByInterruptException if the
current thread is interrupted. Few of the current call sites, nor the APIs they
implement, are designed to allow this kind of fine-grained interrupt handling.

This commit carefully goes through each of the changes in
4b82e6a, redirecting potentially unsafe uses of
Files.newInputStream/newOutputStream to a new class FilesNI, which currently
just creates a regular old FileInputStream/FileOutputStream. An NIO-based
approach, if necessary, can be implemented there at a later point if deemed
worthwhile.

ClosedByInterruptException currently appears in bug reports NETBEANS-609,
NETBEANS-1340, NETBEANS-1061, NETBEANS-920, NETBEANS-583, NETBEANS-1197, and
NETBEANS-1357.

The JarClassLoader case was handled separately in NETBEANS-1197.
@eirikbakke
Copy link
Contributor Author

(Tried to invite mdindoffer as a reviewer, but the dropdown didn't let me select that username.)

@matthiasblaesing
Copy link
Contributor

I did not review yet and I'm currently wondering if it is worth it. With the knowledge of today (underlying issue fixed in JDK, unhandled error situation newly introduced) I would not have merged it. So I openly ask - should we just revert commits:

From my POV we are introducing much code to work-around a non-problem on recent JDKs. In one year no-one can explain what made us introduce org.openide.util.io.FilesNI. git revert does not fully run clean, but manually reverting should be perfectly doable.

I'll tag @mdindoffer here to get his opinion.

@matthiasblaesing
Copy link
Contributor

Ok - this would be the realization of a strait backout of the changesets:

https://github.com/matthiasblaesing/incubator-netbeans/tree/nio-backout

The revert: matthiasblaesing@6b0b3b2 was created manually. An automatic revert failed. It looks as if the cluster reorganisation broke some path tracking. The manually created reverse patch applied with minimal fuzzyness.

@mdindoffer
Copy link
Contributor

Sorry for late response I've been travelling a lot this week.
Considering jdk 11 is out already, I'm fine with reverting the patches.

@matthiasblaesing
Copy link
Contributor

@eirikbakke would you agree to merge the nio-backout branch?

The new API basicly reinstates the old code paths via FileInputStreams and has no way to activate it. Given that the pushing moment (releaving GC pressure) is going away, I doubt the implementation will be done for NIO.

If you agree I would also ask Laszlo to merge it into the 10 release branch.

</description>
<class package="org.openide.util.io" name="FilesNI"/>
<issue number="NETBEANS-583"/>
</change>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use 4 spaces.

@eirikbakke
Copy link
Contributor Author

Sorry, I was also traveling this week! Yes, @matthiasblaesing, I applied revert patch and got the same result as when I did a revert myself. I'm fine with reverting the NIO changes as well. That makes this PR moot once the revert is applied.

matthiasblaesing added a commit to matthiasblaesing/netbeans that referenced this pull request Nov 11, 2018
@matthiasblaesing
Copy link
Contributor

Merged revert. Thank you for checking.

@eirikbakke eirikbakke deleted the NETBEANS-583 branch October 5, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Change [ci] enable extra API related tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants