fix(file-io): Do not leak SentryFileInputStream/SentryFileOutputStream descriptors and channels

Author: romtsn

sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java

@@ -14,18 +14,15 @@
 import io.sentry.protocol.User;
 import io.sentry.samples.android.compose.ComposeActivity;
 import io.sentry.samples.android.databinding.ActivityMainBinding;
-import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.OutputStreamWriter;
-import java.io.Writer;
+import java.nio.channels.FileChannel;
 import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Collections;
 import java.util.List;
-import java.util.Locale;
 import java.util.concurrent.CountDownLatch;
 import timber.log.Timber;
 
@@ -86,16 +83,10 @@ protected void onCreate(Bundle savedInstanceState) {
         view -> {
           String fileName = Calendar.getInstance().getTimeInMillis() + "_file.txt";
           File file = getApplication().getFileStreamPath(fileName);
-          try (final FileOutputStream fileOutputStream = new SentryFileOutputStream(file);
-              final OutputStreamWriter outputStreamWriter =
-                  new OutputStreamWriter(fileOutputStream);
-              final Writer writer = new BufferedWriter(outputStreamWriter)) {
-            for (int i = 0; i < 1024; i++) {
-              // To keep the sample code simple this happens on the main thread. Don't do this in a
-              // real app.
-              writer.write(String.format(Locale.getDefault(), "%d\n", i));
-            }
-            writer.flush();
+          try (final FileOutputStream fis =
+              SentryFileOutputStream.Factory.create(new FileOutputStream(file), file)) {
+            FileChannel channel = fis.getChannel();
+            channel.write(java.nio.ByteBuffer.wrap("Hello, World!".getBytes()));
           } catch (IOException e) {
             Sentry.captureException(e);
           }

sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java

@@ -113,6 +113,7 @@ public long skip(final long n) throws IOException {
   @Override
   public void close() throws IOException {
     spanManager.finish(delegate);
+    super.close();
   }
 
   private static FileDescriptor getFileDescriptor(final @NotNull FileInputStream stream)

sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java

@@ -120,6 +120,7 @@ public void write(final byte @NotNull [] b, final int off, final int len) throws
   @Override
   public void close() throws IOException {
     spanManager.finish(delegate);
+    super.close();
   }
 
   private static FileDescriptor getFileDescriptor(final @NotNull FileOutputStream stream)

sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt

@@ -0,0 +1,54 @@
+package io.sentry
+
+import io.sentry.cache.PersistingScopeObserver
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TemporaryFolder
+
+class PersistingScopeObserverPerformanceTest {
+
+    @get:Rule
+    val tmpDir = TemporaryFolder()
+
+    class Fixture {
+
+        val options = SentryOptions()
+        val scope = Scope(options)
+
+        fun getSut(cacheDir: TemporaryFolder): PersistingScopeObserver {
+            options.run {
+                executorService = SentryExecutorService()
+                cacheDirPath = cacheDir.newFolder().absolutePath
+            }
+            return PersistingScopeObserver(options)
+        }
+    }
+
+    private val fixture = Fixture()
+
+    @Test
+    fun test() {
+        val sut = fixture.getSut(tmpDir)
+
+        var oldDuration = 0L
+        var newDuration = 0L
+
+        val crumb = Breadcrumb.http("https://example.com", "GET", 200)
+
+        for (i in 0..100) {
+            val oldStart = System.currentTimeMillis()
+            for (j in 0..100) {
+                sut.addBreadcrumb(crumb)
+            }
+            oldDuration += System.currentTimeMillis() - oldStart
+
+            val newStart = System.currentTimeMillis()
+            for (j in 0..100) {
+//                sut.addBreadcrumbNew(crumb)
+            }
+            newDuration += System.currentTimeMillis() - newStart
+        }
+
+        println("It took old: ${oldDuration}ms, new: ${newDuration}ms")
+    }
+}

sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt

@@ -18,6 +18,7 @@ import java.io.File
 import java.io.FileDescriptor
 import java.io.FileInputStream
 import java.io.IOException
+import java.nio.ByteBuffer
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.concurrent.thread
 import kotlin.test.Test
@@ -285,6 +286,17 @@ class SentryFileInputStreamTest {
 
         assertTrue { stream is ThrowingFileInputStream }
     }
+
+    @Test
+    fun `channels and descriptors are closed together with the stream`() {
+        val fis = fixture.getSut(tmpFile)
+        val channel = fis.channel
+
+        channel.read(ByteBuffer.allocate(1))
+        fis.close()
+        assertFalse(channel.isOpen)
+        assertFalse(fis.fd.valid())
+    }
 }
 
 class ThrowingFileInputStream(file: File) : FileInputStream(file) {

sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt

@@ -16,6 +16,7 @@ import org.mockito.kotlin.whenever
 import java.io.File
 import java.io.FileOutputStream
 import java.io.IOException
+import java.nio.ByteBuffer
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.concurrent.thread
 import kotlin.test.Test
@@ -231,6 +232,17 @@ class SentryFileOutputStreamTest {
 
         assertTrue { stream is ThrowingFileOutputStream }
     }
+
+    @Test
+    fun `channels and descriptors are closed together with the stream`() {
+        val fos = fixture.getSut(tmpFile)
+        val channel = fos.channel
+
+        channel.write(ByteBuffer.wrap("hello".toByteArray()))
+        fos.close()
+        assertFalse(channel.isOpen)
+        assertFalse(fos.fd.valid())
+    }
 }
 
 class ThrowingFileOutputStream(file: File) : FileOutputStream(file) {