Fix leaking file descriptors when closing

When closing a BufferedWriter, we close the underlying writer. As we're
using channels, this is an instance of sun.nio.cs.StreamEncoder. This
will attempt to flush the pending character.

However, if throwing an exception within .write errors, the flush will
fail and so the underlying stream is not closed. This was causing us to
leak file descriptors.

We fix this by introducing ChannelWrappers - this holds the wrapper
object (say, a BufferedWriter) and underlying channel. When closed, we
dispose of the wrapper, and then the channel. You could think of this as
doing a nested try-with-resources, rather than a single one.

Note, this is not related to JDK-6378948 - this occurs in the underlying
stream encoder instead.

src/main/java/dan200/computercraft/core/filesystem/ChannelWrapper.java

@@ -0,0 +1,50 @@
+/*
+ * This file is part of ComputerCraft - http://www.computercraft.info
+ * Copyright Daniel Ratcliffe, 2011-2019. Do not distribute without permission.
+ * Send enquiries to dratcliffe@gmail.com
+ */
+
+package dan200.computercraft.core.filesystem;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.nio.channels.Channel;
+
+/**
+ * Wraps some closeable object such as a buffered writer, and the underlying stream.
+ *
+ * When flushing a buffer before closing, some implementations will not close the buffer if an exception is thrown
+ * this causes us to release the channel, but not actually close it. This wrapper will attempt to close the wrapper (and
+ * so hopefully flush the channel), and then close the underlying channel.
+ *
+ * @param <T> The type of the closeable object to write.
+ */
+class ChannelWrapper<T extends Closeable> implements Closeable
+{
+    private final T wrapper;
+    private final Channel channel;
+
+    ChannelWrapper( T wrapper, Channel channel )
+    {
+        this.wrapper = wrapper;
+        this.channel = channel;
+    }
+
+    @Override
+    public void close() throws IOException
+    {
+        try
+        {
+            wrapper.close();
+        }
+        finally
+        {
+            channel.close();
+        }
+    }
+
+    public T get()
+    {
+        return wrapper;
+    }
+}

src/main/java/dan200/computercraft/core/filesystem/FileSystem.java

@@ -19,6 +19,7 @@
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
+import java.nio.channels.Channel;
 import java.nio.channels.ReadableByteChannel;
 import java.nio.channels.WritableByteChannel;
 import java.nio.file.AccessDeniedException;
@@ -317,7 +318,7 @@ private String toLocal( String path )
     private final FileSystemWrapperMount m_wrapper = new FileSystemWrapperMount( this );
     private final Map<String, MountWrapper> m_mounts = new HashMap<>();
 
-    private final HashMap<WeakReference<FileSystemWrapper<?>>, Closeable> m_openFiles = new HashMap<>();
+    private final HashMap<WeakReference<FileSystemWrapper<?>>, ChannelWrapper<?>> m_openFiles = new HashMap<>();
     private final ReferenceQueue<FileSystemWrapper<?>> m_openFileQueue = new ReferenceQueue<>();
 
     public FileSystem( String rootLabel, IMount rootMount ) throws FileSystemException
@@ -661,7 +662,7 @@ private void cleanup()
         }
     }
 
-    private synchronized <T extends Closeable> FileSystemWrapper<T> openFile( @Nonnull T file ) throws FileSystemException
+    private synchronized <T extends Closeable> FileSystemWrapper<T> openFile( @Nonnull Channel channel, @Nonnull T file ) throws FileSystemException
     {
         synchronized( m_openFiles )
         {
@@ -669,12 +670,14 @@ private synchronized <T extends Closeable> FileSystemWrapper<T> openFile( @Nonnu
                 m_openFiles.size() >= ComputerCraft.maximumFilesOpen )
             {
                 IoUtil.closeQuietly( file );
+                IoUtil.closeQuietly( channel );
                 throw new FileSystemException( "Too many files already open" );
             }
 
-            FileSystemWrapper<T> wrapper = new FileSystemWrapper<>( this, file, m_openFileQueue );
-            m_openFiles.put( wrapper.self, file );
-            return wrapper;
+            ChannelWrapper<T> channelWrapper = new ChannelWrapper<T>( file, channel );
+            FileSystemWrapper<T> fsWrapper = new FileSystemWrapper<>( this, channelWrapper, m_openFileQueue );
+            m_openFiles.put( fsWrapper.self, channelWrapper );
+            return fsWrapper;
         }
     }
 
@@ -695,7 +698,7 @@ public synchronized <T extends Closeable> FileSystemWrapper<T> openForRead( Stri
         ReadableByteChannel channel = mount.openForRead( path );
         if( channel != null )
         {
-            return openFile( open.apply( channel ) );
+            return openFile( channel, open.apply( channel ) );
         }
         return null;
     }
@@ -709,7 +712,7 @@ public synchronized <T extends Closeable> FileSystemWrapper<T> openForWrite( Str
         WritableByteChannel channel = append ? mount.openForAppend( path ) : mount.openForWrite( path );
         if( channel != null )
         {
-            return openFile( open.apply( channel ) );
+            return openFile( channel, open.apply( channel ) );
         }
         return null;
     }

src/main/java/dan200/computercraft/core/filesystem/FileSystemWrapper.java

@@ -15,18 +15,23 @@
 /**
  * An alternative closeable implementation that will free up resources in the filesystem.
  *
+ * The {@link FileSystem} maps weak references of this to its underlying object. If the wrapper has been disposed of
+ * (say, the Lua object referencing it has gone), then the wrapped object will be closed by the filesystem.
+ *
+ * Closing this will stop the filesystem tracking it, reducing the current descriptor count.
+ *
  * In an ideal world, we'd just wrap the closeable. However, as we do some {@code instanceof} checks
  * on the stream, it's not really possible as it'd require numerous instances.
  *
- * @param <T> The stream to wrap.
+ * @param <T> The type of writer or channel to wrap.
  */
 public class FileSystemWrapper<T extends Closeable> implements Closeable
 {
     private final FileSystem fileSystem;
-    private final T closeable;
+    private final ChannelWrapper<T> closeable;
     final WeakReference<FileSystemWrapper<?>> self;
 
-    FileSystemWrapper( FileSystem fileSystem, T closeable, ReferenceQueue<FileSystemWrapper<?>> queue )
+    FileSystemWrapper( FileSystem fileSystem, ChannelWrapper<T> closeable, ReferenceQueue<FileSystemWrapper<?>> queue )
     {
         this.fileSystem = fileSystem;
         this.closeable = closeable;
@@ -43,6 +48,6 @@ public void close() throws IOException
     @Nonnull
     public T get()
     {
-        return closeable;
+        return closeable.get();
     }
 }

src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java

@@ -213,7 +213,11 @@ public void handleEvent( String eventName, Object[] arguments )
 
             if( m_mainRoutine.getStatus().equals( "dead" ) ) close();
         }
-        catch( LuaError | HardAbortError | InterruptedException e )
+        catch( HardAbortError | InterruptedException e )
+        {
+            close();
+        }
+        catch( LuaError e )
         {
             close();
             ComputerCraft.log.warn( "Top level coroutine errored", e );