From faee555f4ec0d13544629cda21e827d093f3e75a Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 28 Mar 2025 15:16:04 +0100 Subject: [PATCH 1/4] fix(file-io): Do not leak SentryFileInputStream/SentryFileOutputStream descriptors and channels --- .../sentry/samples/android/MainActivity.java | 19 ++----- .../file/SentryFileInputStream.java | 1 + .../file/SentryFileOutputStream.java | 1 + .../PersistingScopeObserverPerformanceTest.kt | 54 +++++++++++++++++++ .../file/SentryFileInputStreamTest.kt | 12 +++++ .../file/SentryFileOutputStreamTest.kt | 12 +++++ 6 files changed, 85 insertions(+), 14 deletions(-) create mode 100644 sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java index e881612bd80..d1daba56261 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java +++ b/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); } diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java index 0ee5df6d799..3119b50b5ed 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java +++ b/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) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java index 483d6e97819..76f4cd68cba 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java +++ b/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) diff --git a/sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt b/sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt new file mode 100644 index 00000000000..f9c311eb708 --- /dev/null +++ b/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") + } +} diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt index db52296471f..e0b9316a239 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt +++ b/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) { diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt index df66a3b6c30..ed7edb5785e 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt +++ b/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) { From bfde3d47e7ad1d785ede59bcb6862ea56b96f800 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 28 Mar 2025 15:17:24 +0100 Subject: [PATCH 2/4] Revert --- .../PersistingScopeObserverPerformanceTest.kt | 54 ------------------- 1 file changed, 54 deletions(-) delete mode 100644 sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt diff --git a/sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt b/sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt deleted file mode 100644 index f9c311eb708..00000000000 --- a/sentry/src/test/java/io/sentry/PersistingScopeObserverPerformanceTest.kt +++ /dev/null @@ -1,54 +0,0 @@ -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") - } -} From 278fb2d520a03b34f0d187e567ca954b978aa690 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 28 Mar 2025 15:20:19 +0100 Subject: [PATCH 3/4] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d136fd9c7a..b027fd7834b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ - An example value would be `8.6.0` - The value of the `Sentry-Version-Name` attribute looks like `sentry-8.5.0-otel-2.10.0` - Fix tags missing for compose view hierarchies ([#4275](https://github.com/getsentry/sentry-java/pull/4275)) +- Do not leak SentryFileInputStream/SentryFileOutputStream descriptors and channels ([#4296](https://github.com/getsentry/sentry-java/pull/4296)) ### Internal From b67116568a98f0ceec3eb3fa09ab37ca47fa7fed Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Mon, 31 Mar 2025 12:05:18 +0200 Subject: [PATCH 4/4] Update sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java Co-authored-by: Stefano --- .../src/main/java/io/sentry/samples/android/MainActivity.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java index d1daba56261..a4085bf8225 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java @@ -83,9 +83,9 @@ protected void onCreate(Bundle savedInstanceState) { view -> { String fileName = Calendar.getInstance().getTimeInMillis() + "_file.txt"; File file = getApplication().getFileStreamPath(fileName); - try (final FileOutputStream fis = + try (final FileOutputStream fos = SentryFileOutputStream.Factory.create(new FileOutputStream(file), file)) { - FileChannel channel = fis.getChannel(); + FileChannel channel = fos.getChannel(); channel.write(java.nio.ByteBuffer.wrap("Hello, World!".getBytes())); } catch (IOException e) { Sentry.captureException(e);