From 62be2d5d3549d9a3624dc36bac42429735c88d3f Mon Sep 17 00:00:00 2001 From: Jason Date: Mon, 17 Oct 2022 16:19:46 +0100 Subject: [PATCH] fix(anr): immediately run any tasks submitted to the same background queue submitting the work to avoid possible thread starvation --- CHANGELOG.md | 9 +++ .../bugsnag/android/BackgroundTaskService.kt | 64 +++++++++++++++++-- .../android/BackgroundTaskServiceTest.kt | 21 ++++++ 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eddfe9ae0d..6a4b75d506 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## TBD + +### Bug fixes + +* Fixed rare thread-starvation issue where some internal failures could lead to deadlocks. This was most noticeable + when attempting to call Bugsnag.start on an architecture (ABI) that was not packaged in the APK, and lead to an + ANR instead of an error report. + []() + ## 5.28.0 (2022-10-13) ### Enhancements diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt index c171c23d50..9f4090f731 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt @@ -3,6 +3,7 @@ package com.bugsnag.android import androidx.annotation.VisibleForTesting import java.util.concurrent.BlockingQueue import java.util.concurrent.Callable +import java.util.concurrent.ExecutionException import java.util.concurrent.Executors import java.util.concurrent.Future import java.util.concurrent.LinkedBlockingQueue @@ -10,6 +11,7 @@ import java.util.concurrent.RejectedExecutionException import java.util.concurrent.ThreadFactory import java.util.concurrent.ThreadPoolExecutor import java.util.concurrent.TimeUnit +import java.lang.Thread as JThread /** * The type of task which is being submitted. This determines which execution queue @@ -55,9 +57,14 @@ private const val THREAD_POOL_SIZE = 1 private const val KEEP_ALIVE_SECS = 30L private const val TASK_QUEUE_SIZE = 128 -internal fun createExecutor(name: String, keepAlive: Boolean): ThreadPoolExecutor { +private class TaskTypeThread(runnable: Runnable, name: String, val taskType: TaskType) : + JThread(runnable, name) + +internal val JThread.taskType get() = (this as? TaskTypeThread)?.taskType + +internal fun createExecutor(name: String, type: TaskType, keepAlive: Boolean): ThreadPoolExecutor { val queue: BlockingQueue = LinkedBlockingQueue(TASK_QUEUE_SIZE) - val threadFactory = ThreadFactory { Thread(it, name) } + val threadFactory = ThreadFactory { TaskTypeThread(it, name, type) } // certain executors (error/session/io) should always keep their threads alive, but others // are less important so are allowed a pool size of 0 that expands on demand. @@ -86,33 +93,38 @@ internal fun createExecutor(name: String, keepAlive: Boolean): ThreadPoolExecuto internal class BackgroundTaskService( // these executors must remain single-threaded - the SDK makes assumptions // about synchronization based on this. - @VisibleForTesting + @get:VisibleForTesting internal val errorExecutor: ThreadPoolExecutor = createExecutor( "Bugsnag Error thread", + TaskType.ERROR_REQUEST, true ), - @VisibleForTesting + @get:VisibleForTesting internal val sessionExecutor: ThreadPoolExecutor = createExecutor( "Bugsnag Session thread", + TaskType.SESSION_REQUEST, true ), - @VisibleForTesting + @get:VisibleForTesting internal val ioExecutor: ThreadPoolExecutor = createExecutor( "Bugsnag IO thread", + TaskType.IO, true ), - @VisibleForTesting + @get:VisibleForTesting internal val internalReportExecutor: ThreadPoolExecutor = createExecutor( "Bugsnag Internal Report thread", + TaskType.INTERNAL_REPORT, false ), - @VisibleForTesting + @get:VisibleForTesting internal val defaultExecutor: ThreadPoolExecutor = createExecutor( "Bugsnag Default thread", + TaskType.DEFAULT, false ) ) { @@ -138,6 +150,19 @@ internal class BackgroundTaskService( */ @Throws(RejectedExecutionException::class) fun submitTask(taskType: TaskType, callable: Callable): Future { + /* + * Our executors are all single-threaded queues. Whenever a task is submitted we check to + * see if its being submitted to the current thread, if it is then we run it immediately + * and return to avoid thread star + */ + if (JThread.currentThread().taskType == taskType) { + return try { + CompletedFuture(callable.call()) + } catch (ex: Throwable) { + FailedFuture(ex) + } + } + return when (taskType) { TaskType.ERROR_REQUEST -> errorExecutor.submit(callable) TaskType.SESSION_REQUEST -> sessionExecutor.submit(callable) @@ -175,4 +200,29 @@ internal class BackgroundTaskService( // ignore interrupted exception as the JVM is shutting down } } + + private class CompletedFuture(private val value: V) : Future { + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + override fun isCancelled(): Boolean = false + override fun isDone(): Boolean = true + + override fun get(): V = value + override fun get(timeout: Long, unit: TimeUnit?): V = value + } + + private class FailedFuture(private val failure: Throwable) : Future { + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + override fun isCancelled(): Boolean = false + override fun isDone(): Boolean = true + + @Throws(ExecutionException::class) + override fun get(): V { + throw ExecutionException(failure) + } + + @Throws(ExecutionException::class) + override fun get(timeout: Long, unit: TimeUnit?): V { + throw ExecutionException(failure) + } + } } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/BackgroundTaskServiceTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/BackgroundTaskServiceTest.kt index b67f798e51..2e071a13fc 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/BackgroundTaskServiceTest.kt +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/BackgroundTaskServiceTest.kt @@ -136,6 +136,27 @@ internal class BackgroundTaskServiceTest { assertRejectedExecution(service, TaskType.IO) } + /** + * Test that tasks submitted to the same queue within a task work without deadlocking the queue. + * This has a 5 second timeout since the symptom of a failure is thread-starvation / deadlock. + */ + @Test(timeout = 5_000) + fun testSubmitOnSubmit() { + val service = BackgroundTaskService() + + TaskType.values().forEach { taskType -> + val result = service.submitTask>(taskType) { + service.submitTask>(taskType) { + taskType to "done" + }.get() + }.get() + + assertEquals(taskType to "done", result) + } + + service.shutdown() + } + private fun submitBlockingJob( service: BackgroundTaskService, latch: CountDownLatch,