Fix class loading error when a thread is interrupted

[Yeregorix]

When a thread is interrupted while loading a class from disk, the underlying zip filesystem is closed. Any further class loads from the same module will fail.

How to reproduce:

new Thread(() -> {
	Thread.currentThread().interrupt();
	AnyClassNotLoadedYet.class.getName();
}).start();

This will result in a ClassNotFoundException.

By adding printStackTrace() in SecureModuleClassLoader#findClass(module, name) I can get the following stacktrace:

java.nio.channels.ClosedByInterruptException
	at java.base/java.nio.channels.spi.AbstractInterruptibleChannel.end(AbstractInterruptibleChannel.java:200)
	at java.base/sun.nio.ch.FileChannelImpl.endBlocking(FileChannelImpl.java:172)
	at java.base/sun.nio.ch.FileChannelImpl.readInternal(FileChannelImpl.java:992)
	at java.base/sun.nio.ch.FileChannelImpl.read(FileChannelImpl.java:964)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1241)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1236)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.initDataPos(ZipFileSystem.java:2386)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.read(ZipFileSystem.java:2328)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$2.fill(ZipFileSystem.java:2278)
	at java.base/java.util.zip.InflaterInputStream.read(InflaterInputStream.java:175)
	at java.base/java.io.InputStream.readNBytes(InputStream.java:412)
	at java.base/java.io.InputStream.readAllBytes(InputStream.java:349)
	at cpw.mods.securejarhandler/net.minecraftforge.securemodules.SecureModuleClassLoader.getClassBytes(SecureModuleClassLoader.java:182)
	at cpw.mods.securejarhandler/cpw.mods.cl.ModuleClassLoader.getClassBytes(ModuleClassLoader.java:38)
	at cpw.mods.securejarhandler/net.minecraftforge.securemodules.SecureModuleClassLoader.readerToClass(SecureModuleClassLoader.java:476)
	at cpw.mods.securejarhandler/net.minecraftforge.securemodules.SecureModuleClassLoader.findClass(SecureModuleClassLoader.java:406)
	at cpw.mods.securejarhandler/net.minecraftforge.securemodules.SecureModuleClassLoader.loadClass(SecureModuleClassLoader.java:423)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
	at TRANSFORMER/test_plugin@1.0.0/net.smoofyuniverse.test.TestPlugin.lambda$onServerStarting$1(TestPlugin.java:86)
	at java.base/java.lang.Thread.run(Thread.java:1583)

This error already existed in the past and was fixed by commit 4e05cab. However it has been fixed only for the union filesystem. Since zip filesystem is currently preferred when possible, this issue still occurs.

In this PR, I solve this issue by always preferring the union filesystem, even when there is a single path.

[LexManos]

Reverting to the performance degradation of union file system is not a valid solution.

The eventual goal is to strip out union file system into its own project and only have it avaliable at dev time.

What actually closes the file system?

If this is an issue with the ZipFileSystem then it would lead me to think this is a java bug and should be reported to them. If this is not an issue in the build in classloader. Then im curious how it deals with this case.

Maybe a better solution is to open the file system in a way that blocks other threads from closing it. Or handling this edge case and reopening the filesystem.

Is there a way to reliably write a unit test for this?
I'm out of town for the 4th. So it'll be a bit before I can look into this myself.

[Yeregorix]

For context, I discovered the issue when using the mongodb-driver-sync library to connect to a database. When I close the connection, the library interrupts the threads in its thread pool and then the classloader explodes.

The zip filesystem is backed internally by a FileChannel, which extends AbstractInterruptibleChannel. When the thread is interrupted, the channel closes and operations throw a ClosedByInterruptException. I don't think it's a Java bug, just the standard behavior as documented in ClosedByInterruptException javadoc.

It is possible to disable this behavior by calling setUninterruptible on the file channel. This is what the union filesystem does: https://github.com/MinecraftForge/SecureModules/blob/master/src/main/java/cpw/mods/niofs/union/UnionFileSystem.java#L147.

So an alternative solution is to do the same trick on a standard zip filesystem.

[LexManos]

That would indeed be the preferred option then.
Again I'll have to look at it when im back in town. But the eventual goal is to get ride of UnionFileSystem. It has many hacky behaviors and performance issues.
I am not particularly happy with the existing system I use to open the ZipFileSystem (see the comments about internal caching) but it's good for now. Adding one smaller hack isnt a deal breaker.

Also, what i mean by java bug is the overall behavior of classes not being able to be loaded from a zip if one thread gets interrupted. How does normal java deal with that? Is it an issue that arrives with the standard classloader. If not. Why not.

[Yeregorix]

Should setUninterruptible be called on the cached zip fs? Or on an fresh and independent one? Since the cached one is shared, should we worry about potential side-effects?

How does normal java deal with that? Is it an issue that arrives with the standard classloader. If not. Why not.

I just took a look at standard classloaders, URLClassLoader is backed by URLConnection. AppClassLoader is backed by a mix of URLConnection and ZipFile. No FileChannel seems to be involved.

[LexManos]

Should setUninterruptible be called on the cached zip fs? Or on an fresh and independent one? Since the cached one is shared, should we worry about potential side-effects?

Is there any way to get our own ZipFileSystem without the duplicate file system issue?
I don't think there is, so calling it on the cached ZFS would be needed.

A URLConnection should have the same issues as i think it's based by ZFS (hence using a url to force the FS cache)

Another potentially stupid solution would be to see what it takes to support ZipFile in addition to the NIO system. So that we can skip NIO for the common usecase.

I am.unsure of the feasibility or best route to go. But the options are worth looking into if/when someone has time.

[Yeregorix]

I investigated a bit more, I was wrong. URLConnection is only the fallback when protocol is unknown. When protocol is jar the standard classloader uses a direct JarFile and its ZipFileInputStream. So no ZFS.

Anyway, I updated the PR to have a unit test and the discussed solution.

[LexManos]

I investigated a bit more, I was wrong. URLConnection is only the fallback when protocol is unknown. When protocol is jar the standard classloader uses a direct JarFile and its ZipFileInputStream. So no ZFS.

Ah, interesting

Anyway, I updated the PR to have a unit test and the discussed solution.

I shall take a look when I get a chance. Thanks. No "new" hacks. Just moving some of the existing. And doesn't use UFS.

[LexManos]

Okay so I'm back in town and alive enough to look into this.
Was able to figure out where and what the issues are. And unfortunately, there are no good solutions, the proper solution would be a complete re-work of the Jar/SecureJar API to not expose the FileSystem itself. Instead to just expose ModuleReader and related interfaces that the JVM itself uses when dealing with modules.
Doing that would allow us to have different implementations in the backend like Java Does
(See JarModuleReader and JModModuleReader)

But that would be a big refactor/breaking change that I need to think about more.

This PR looks fine, Not happy that it does a hack, but its a hack that the UFS has been doing for years so guess it works. I'm gunna add a comment linking to this PR in the util class for Future Me to remember when designing a breaking change.