> No, not quite ignoring them. The thread's interrupt bit is left on after the > operation.
The point is, a thread got interrupted, and instead of stopping (expected behaviour of a well-known contract) we force-retry I/O reads. Leaving an interrupt flag on is just akin to leaving a message in the error log. Not stopping == ignoring. Maybe the retry scenario is OK for the `JarClassLoader`, but applying this workaround globally is just an antipattern. > This is a valid way to handle interrupts, when the calling API provides no > way to throw an InterruptedException, or otherwise communicate a canceled > operation It has a way to communicate the canceled operation. That's what's the `ClosedByInterruptException` for. > In the JarClassLoader case, there was no incorrect handling prior to the > change. >It used FileInputStream/FileOutputStream, which cannot throw a >ClosedByInterruptException, and so wouldn't be expected to handle it. The >switch to NIO makes it necessary to handle it specially. Ideally, this special >handling would be to immediately stop the classloading process--but _a lot_ of >care would have to be taken to make sure this doesn't introduce new, subtle >bugs. In the end, the loaded class would likely be cached in any case, so even >though the current thread was interrupted, the extra time it took to load the >class would not be wasted. > > Another example is FileObject.getInputStream(). Looking at stack traces, I > suspect this public API is now returning a NIO stream instead of a > FileInputStream as a result of the NIO patch (am I right?). The fact that the > returned InputStream's various I/O methods can now throw a > ClosedByInterruptException, which needs to be handled differently than other > IOException exceptions, amounts to a backwards-incompatible API change. > Clients should not be expected to handle specially unless they themselves > makes use of the new NIO streams. > > > Or incorrect interrupts, depends on the point of view. > > I would avoid this approach--library code like this should not break if > client code interrupts the current thread. Thread interruption is a normal > feature of the threading API that client code (including platform application > and plugin code that we do not control) should be able to use as they see > fit. And it worked in the past, and so should not break in the future. I see your point about breaking the API. I guess it comes down to whether you want to do it the right way - fix the callers / interrupters - or mask the interrupts - which is what the old IO API did. If you want to revert 4b82e6a, go ahead. If you want to use workarounds such as this PR, go ahead as well. Just know it means relying on an implementation detail and can change in the future. And workarounds as this one are deliberately ignoring the interrupt contract of NIO. Hence I'm for fixing the callers properly. [ Full content available at: https://github.com/apache/incubator-netbeans/pull/854 ] This message was relayed via gitbox.apache.org for [email protected]
