fix(database,android): fail a transaction whose handler never replies

A transaction whose Dart handler did not reply within 30 seconds, or was
released because the plugin detached from its engine, resolved with
`committed: false` and no error, exactly like a handler that returned
`Transaction.abort()`. A debugger pause longer than the timeout silently
aborted valid transactions.

`runTransaction` now throws a `FirebaseException` instead: code
`deadline-exceeded` on timeout (as cloud_firestore does), or `cancelled`
when the engine detached first. The failure is recorded on the handler
and reported from onComplete, because the RTDB SDK turns an exception
thrown from doTransaction into an uncoded "user code" error.

Also:
- reset the detached state when the plugin is attached again, so a
  re-attached plugin calls Dart again instead of aborting every
  transaction;
- use a reply that arrives just after the timeout, and only log when the
  result is really missing;
- correct the comment on the timeout, which is reached by debugger
  pauses, blocked main threads and replies lost during hot restart or
  detach.

Author: Lyokone

packages/firebase_database/firebase_database/android/src/main/kotlin/io/flutter/plugins/firebase/database/FirebaseDatabasePlugin.kt

@@ -52,8 +52,10 @@ internal constructor(
     private const val METHOD_CHANNEL_NAME = "plugins.flutter.io/firebase_database"
     private val databaseInstanceCache = HashMap<String, FirebaseDatabase>()
 
-    // The Dart handler is synchronous, so this only fires when no reply is coming. It matches the
-    // default transaction timeout of cloud_firestore.
+    // How long a transaction waits for its Dart handler, which is synchronous. It is reached when
+    // the isolate is paused in a debugger, the main thread is blocked, or the reply is lost, as
+    // during a hot restart or once the engine is detached. It matches the default transaction
+    // timeout of cloud_firestore.
     private val TRANSACTION_HANDLER_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(30)
   }
 
@@ -82,7 +84,9 @@ internal constructor(
     }
   }
 
-  private fun initPluginInstance(messenger: BinaryMessenger) {
+  internal fun initPluginInstance(messenger: BinaryMessenger) {
+    // The plugin can be attached again after a detach.
+    isDetached = false
     FlutterFirebasePluginRegistry.registerPlugin(METHOD_CHANNEL_NAME, this)
     this.messenger = messenger
 
@@ -457,6 +461,10 @@ internal constructor(
   override fun onDetachedFromEngine(
       @NonNull binding: FlutterPluginBinding,
   ) {
+    detachFromEngine()
+  }
+
+  internal fun detachFromEngine() {
     isDetached = true
     pendingTransactionHandlers.forEach { it.countDown() }
     methodChannel.setMethodCallHandler(null)
@@ -807,51 +815,7 @@ internal constructor(
       transactionRequests[request.transactionKey] = request
 
       // Start the transaction - simplified approach like iOS
-      reference.runTransaction(
-          object : com.google.firebase.database.Transaction.Handler {
-            override fun doTransaction(
-                mutableData: com.google.firebase.database.MutableData
-            ): com.google.firebase.database.Transaction.Result {
-              val result =
-                  callTransactionHandler(request.transactionKey, mutableData.value)
-                      ?: return com.google.firebase.database.Transaction.abort()
-
-              if (result.aborted || result.exception) {
-                return com.google.firebase.database.Transaction.abort()
-              }
-
-              mutableData.value = result.value
-              return com.google.firebase.database.Transaction.success(mutableData)
-            }
-
-            override fun onComplete(
-                error: com.google.firebase.database.DatabaseError?,
-                committed: Boolean,
-                currentData: com.google.firebase.database.DataSnapshot?
-            ) {
-              // Store the transaction result for later retrieval
-              val result =
-                  mapOf(
-                      "committed" to committed,
-                      "snapshot" to
-                          mapOf(
-                              "value" to currentData?.value,
-                              "key" to currentData?.key,
-                              "exists" to currentData?.exists()))
-              transactionResults[request.transactionKey] = result
-
-              // Complete the transaction - simplified like iOS
-              if (error != null) {
-                val ex = FlutterFirebaseDatabaseException.fromDatabaseError(error)
-                callback(
-                    KotlinResult.failure(
-                        FlutterError("firebase_database", ex.message, ex.additionalData)))
-              } else {
-                callback(KotlinResult.success(Unit))
-              }
-            }
-          },
-          request.applyLocally)
+      reference.runTransaction(transactionHandler(request, callback), request.applyLocally)
     } catch (e: Exception) {
       // Convert generic exceptions to FlutterFirebaseDatabaseException for proper error handling
       val flutterException =
@@ -1059,6 +1023,65 @@ internal constructor(
     return database
   }
 
+  /** Bridges the RTDB transaction of [request] to its Dart handler, and reports to [callback]. */
+  internal fun transactionHandler(
+      request: TransactionRequest,
+      callback: (KotlinResult<Unit>) -> Unit
+  ): com.google.firebase.database.Transaction.Handler =
+      object : com.google.firebase.database.Transaction.Handler {
+        // Why the Dart handler gave no result. Reported from onComplete rather than thrown, because
+        // the SDK turns an exception thrown here into an uncoded "user code" error.
+        @Volatile private var handlerFailure: FlutterFirebaseDatabaseException? = null
+
+        override fun doTransaction(
+            mutableData: com.google.firebase.database.MutableData
+        ): com.google.firebase.database.Transaction.Result {
+          val result =
+              try {
+                callTransactionHandler(request.transactionKey, mutableData.value)
+              } catch (e: FlutterFirebaseDatabaseException) {
+                handlerFailure = e
+                return com.google.firebase.database.Transaction.abort()
+              }
+
+          if (result.aborted || result.exception) {
+            return com.google.firebase.database.Transaction.abort()
+          }
+
+          mutableData.value = result.value
+          return com.google.firebase.database.Transaction.success(mutableData)
+        }
+
+        override fun onComplete(
+            error: com.google.firebase.database.DatabaseError?,
+            committed: Boolean,
+            currentData: com.google.firebase.database.DataSnapshot?
+        ) {
+          // Store the transaction result for later retrieval
+          val result =
+              mapOf(
+                  "committed" to committed,
+                  "snapshot" to
+                      mapOf(
+                          "value" to currentData?.value,
+                          "key" to currentData?.key,
+                          "exists" to currentData?.exists()))
+          transactionResults[request.transactionKey] = result
+
+          // Complete the transaction - simplified like iOS
+          val ex =
+              handlerFailure
+                  ?: error?.let { FlutterFirebaseDatabaseException.fromDatabaseError(it) }
+          if (ex != null) {
+            callback(
+                KotlinResult.failure(
+                    FlutterError("firebase_database", ex.message, ex.additionalData)))
+          } else {
+            callback(KotlinResult.success(Unit))
+          }
+        }
+      }
+
   // Dart transaction handlers that RTDB run-loop threads are waiting on.
   private val pendingTransactionHandlers: MutableSet<CountDownLatch> = ConcurrentHashMap.newKeySet()
   @Volatile private var isDetached = false
@@ -1070,34 +1093,42 @@ internal constructor(
    * until it returns. Nothing replies when the engine is destroyed or its messenger detached while
    * the handler is pending, so the wait times out, and [onDetachedFromEngine] releases it at once.
    *
-   * @return the handler's result, or `null` to abort the transaction.
+   * @throws FlutterFirebaseDatabaseException `deadline-exceeded` when Dart does not reply in time,
+   *   or `cancelled` when the plugin is detached from its engine first.
    */
-  internal fun callTransactionHandler(
-      transactionKey: Long,
-      value: Any?
-  ): TransactionHandlerResult? {
+  internal fun callTransactionHandler(transactionKey: Long, value: Any?): TransactionHandlerResult {
     val latch = CountDownLatch(1)
     pendingTransactionHandlers.add(latch)
     try {
       // Checked after registering the latch, so a concurrent detach either sees it or is seen here.
-      if (isDetached) {
-        return null
-      }
-      val transactionResult = AtomicReference<TransactionHandlerResult?>()
-      // FlutterJNI must be called on the main thread.
-      mainThreadExecutor.execute {
-        FirebaseDatabaseFlutterApi(messenger).callTransactionHandler(transactionKey, value) {
-          transactionResult.set(
-              it.getOrElse {
-                TransactionHandlerResult(value = null, aborted = true, exception = true)
-              })
-          latch.countDown()
+      if (!isDetached) {
+        val transactionResult = AtomicReference<TransactionHandlerResult?>()
+        // FlutterJNI must be called on the main thread.
+        mainThreadExecutor.execute {
+          FirebaseDatabaseFlutterApi(messenger).callTransactionHandler(transactionKey, value) {
+            transactionResult.set(
+                it.getOrElse {
+                  TransactionHandlerResult(value = null, aborted = true, exception = true)
+                })
+            latch.countDown()
+          }
+        }
+        latch.await(transactionHandlerTimeoutMillis, TimeUnit.MILLISECONDS)
+        // Read after the wait, so a reply that arrives just after the timeout is still used.
+        transactionResult.get()?.let {
+          return it
         }
       }
-      if (!latch.await(transactionHandlerTimeoutMillis, TimeUnit.MILLISECONDS)) {
-        Log.w(TAG, "The Dart transaction handler did not reply, aborting the transaction.")
+      if (isDetached) {
+        throw FlutterFirebaseDatabaseException(
+            "cancelled",
+            "The transaction was cancelled because the Flutter engine was detached before its " +
+                "handler replied.")
       }
-      return transactionResult.get()
+      Log.w(TAG, "The Dart transaction handler did not reply in time, failing the transaction.")
+      throw FlutterFirebaseDatabaseException(
+          "deadline-exceeded",
+          "The transaction handler did not reply within $transactionHandlerTimeoutMillis ms.")
     } finally {
       pendingTransactionHandlers.remove(latch)
     }

packages/firebase_database/firebase_database/android/src/test/kotlin/io/flutter/plugins/firebase/database/FirebaseDatabasePluginTest.kt

@@ -4,7 +4,10 @@
 
 package io.flutter.plugins.firebase.database
 
-import io.flutter.embedding.engine.plugins.FlutterPlugin.FlutterPluginBinding
+import com.google.firebase.database.InternalHelpers
+import com.google.firebase.database.MutableData
+import com.google.firebase.database.Transaction
+import com.google.firebase.database.snapshot.NodeUtilities
 import io.flutter.plugin.common.BinaryMessenger
 import io.flutter.plugin.common.BinaryMessenger.BinaryMessageHandler
 import io.flutter.plugin.common.BinaryMessenger.BinaryReply
@@ -14,9 +17,10 @@ import java.util.concurrent.Executors
 import java.util.concurrent.Future
 import java.util.concurrent.LinkedBlockingQueue
 import java.util.concurrent.TimeUnit
+import kotlin.Result as KotlinResult
 import org.junit.After
 import org.junit.Assert.assertEquals
-import org.junit.Assert.assertNull
+import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -37,15 +41,23 @@ class FirebaseDatabasePluginTest {
     override fun setMessageHandler(channel: String, handler: BinaryMessageHandler?) {}
   }
 
+  /** A transaction as the RTDB SDK drives it, with what the plugin reports to Dart. */
+  private inner class TransactionRun(plugin: FirebaseDatabasePlugin) {
+    var reported: KotlinResult<Unit>? = null
+    val handler =
+        plugin.transactionHandler(
+            TransactionRequest(path = "counter", transactionKey = 1, applyLocally = true)) {
+              reported = it
+            }
+    val mutableData: MutableData =
+        InternalHelpers.createMutableData(NodeUtilities.NodeFromJSON("current"))
+
+    /** Runs the handler on a separate thread, as the RTDB SDK does from its run loop. */
+    fun doTransactionOnRunLoop(): Future<Transaction.Result> =
+        rtdbRunLoop.submit<Transaction.Result> { handler.doTransaction(mutableData) }
+  }
+
   private val messenger = FakeDartMessenger()
-  // The plugin only uses the messenger. The constructor is called reflectively because Kotlin
-  // rejects null for its other, @NonNull, parameters.
-  private val binding =
-      FlutterPluginBinding::class
-          .java
-          .constructors
-          .single()
-          .newInstance(null, null, messenger, null, null, null, null) as FlutterPluginBinding
   private val rtdbRunLoop = Executors.newSingleThreadExecutor()
 
   @After
@@ -56,54 +68,97 @@ class FirebaseDatabasePluginTest {
   private fun attachedPlugin(timeoutMillis: Long): FirebaseDatabasePlugin {
     // Runs the main-thread work inline, so the fake Dart side receives calls synchronously.
     val plugin = FirebaseDatabasePlugin(Executor { it.run() }, timeoutMillis)
-    plugin.onAttachedToEngine(binding)
+    plugin.initPluginInstance(messenger)
     return plugin
   }
 
-  /** Calls the handler from a separate thread, as the RTDB SDK does from its run loop. */
-  private fun callTransactionHandlerOnRunLoop(
-      plugin: FirebaseDatabasePlugin
-  ): Future<TransactionHandlerResult?> =
-      rtdbRunLoop.submit<TransactionHandlerResult?> { plugin.callTransactionHandler(1, "current") }
+  private fun replyFromDart(result: TransactionHandlerResult) {
+    messenger.pendingReplies
+        .poll(5, TimeUnit.SECONDS)!!
+        .reply(FirebaseDatabaseFlutterApi.codec.encodeMessage(listOf(result))?.apply { rewind() })
+  }
 
-  @Test
-  fun callTransactionHandler_returnsTheResultFromDart() {
-    val plugin = attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(1))
+  /** Asserts that Dart gets a `FirebaseException` with [code]. */
+  private fun assertReportedFailure(code: String, reported: KotlinResult<Unit>?) {
+    val error = reported!!.exceptionOrNull() as FlutterError
+    // The Dart side reads the code of a `FirebaseException` from the details.
+    assertEquals("firebase_database", error.code)
+    assertEquals(code, (error.details as Map<*, *>)["code"])
+  }
 
-    val pending = callTransactionHandlerOnRunLoop(plugin)
-    val reply = messenger.pendingReplies.poll(5, TimeUnit.SECONDS)!!
-    val dartResult = TransactionHandlerResult(value = "updated", aborted = false, exception = false)
-    reply.reply(
-        FirebaseDatabaseFlutterApi.codec.encodeMessage(listOf(dartResult))?.apply { rewind() })
+  @Test(timeout = 10_000)
+  fun transaction_commitsTheValueFromDart() {
+    val run = TransactionRun(attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(1)))
+
+    val pending = run.doTransactionOnRunLoop()
+    replyFromDart(TransactionHandlerResult(value = "updated", aborted = false, exception = false))
 
-    assertEquals(dartResult, pending.get(5, TimeUnit.SECONDS))
+    assertTrue(pending.get().isSuccess)
+    assertEquals("updated", run.mutableData.value)
+    run.handler.onComplete(null, true, null)
+    assertTrue(run.reported!!.isSuccess)
   }
 
   @Test(timeout = 10_000)
-  fun callTransactionHandler_abortsWhenDartNeverReplies() {
-    val plugin = attachedPlugin(timeoutMillis = 100)
+  fun transaction_abortedByDartCompletesWithoutAnError() {
+    val run = TransactionRun(attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(1)))
+
+    val pending = run.doTransactionOnRunLoop()
+    replyFromDart(TransactionHandlerResult(value = null, aborted = true, exception = false))
 
-    assertNull(callTransactionHandlerOnRunLoop(plugin).get())
+    assertFalse(pending.get().isSuccess)
+    run.handler.onComplete(null, false, null)
+    // Dart reads `committed: false`.
+    assertTrue(run.reported!!.isSuccess)
   }
 
   @Test(timeout = 10_000)
-  fun onDetachedFromEngine_releasesAPendingTransactionHandler() {
+  fun transaction_failsWithDeadlineExceededWhenDartNeverReplies() {
+    val run = TransactionRun(attachedPlugin(timeoutMillis = 100))
+
+    assertFalse(run.doTransactionOnRunLoop().get().isSuccess)
+    run.handler.onComplete(null, false, null)
+
+    assertReportedFailure("deadline-exceeded", run.reported)
+  }
+
+  @Test(timeout = 10_000)
+  fun detachFromEngine_failsAPendingTransaction() {
     val plugin = attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(10))
+    val run = TransactionRun(plugin)
 
-    val pending = callTransactionHandlerOnRunLoop(plugin)
+    val pending = run.doTransactionOnRunLoop()
     // Dart received the call, and the engine goes away before it replies.
     messenger.pendingReplies.take()
-    plugin.onDetachedFromEngine(binding)
+    plugin.detachFromEngine()
 
-    assertNull(pending.get())
+    assertFalse(pending.get().isSuccess)
+    run.handler.onComplete(null, false, null)
+    assertReportedFailure("cancelled", run.reported)
   }
 
   @Test(timeout = 10_000)
-  fun callTransactionHandler_abortsWithoutCallingDartAfterDetach() {
+  fun transaction_failsWithoutCallingDartAfterDetach() {
     val plugin = attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(10))
-    plugin.onDetachedFromEngine(binding)
+    plugin.detachFromEngine()
+    val run = TransactionRun(plugin)
 
-    assertNull(callTransactionHandlerOnRunLoop(plugin).get())
+    assertFalse(run.doTransactionOnRunLoop().get().isSuccess)
     assertTrue(messenger.pendingReplies.isEmpty())
+    run.handler.onComplete(null, false, null)
+    assertReportedFailure("cancelled", run.reported)
+  }
+
+  @Test(timeout = 10_000)
+  fun initPluginInstance_callsDartAgainAfterADetach() {
+    val plugin = attachedPlugin(timeoutMillis = TimeUnit.MINUTES.toMillis(1))
+    plugin.detachFromEngine()
+    plugin.initPluginInstance(messenger)
+    val run = TransactionRun(plugin)
+
+    val pending = run.doTransactionOnRunLoop()
+    replyFromDart(TransactionHandlerResult(value = "updated", aborted = false, exception = false))
+
+    assertTrue(pending.get().isSuccess)
   }
 }