database-ktx: add callbackFlow for eventlisteners by thatfiredev · Pull Request #4012 · firebase/firebase-android-sdk

thatfiredev · 2022-08-19T15:50:16Z

Part of go/kotlin-flows-firebase-android

This PR should add callbackFlows for RTDB's ValueEventListener and ChildEventListener.

google-oss-bot · 2022-08-19T15:54:15Z

Coverage Report ¹

Affected Products

firebase-database-ktx
Overall coverage changed from 77.78% (4f24be3) to 15.56% (65c85df) by -62.22%.
Filename Base (4f24be3) Merge (65c85df) Diff
ChildEvent.kt ? 0.00% ?
Database.kt 77.78% 17.07% -60.70%

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/IOdJ1lMy82.html

github-actions · 2022-08-19T15:56:07Z

Unit Test Results

  4 files -   391   4 suites - 391 14s ⏱️ - 16m 31s
13 tests - 4 702 13 ✔️ - 4 678 0 💤 - 22 0 ❌ - 2
13 runs - 4 718 13 ✔️ - 4 694 0 💤 - 22 0 ❌ - 2

Results for commit b2b7b7b. ± Comparison against base commit 4f24be3.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-08-19T16:21:28Z

Size Report ¹

Affected Products

firebase-database-ktx
Type Base (4f24be3) Merge (65c85df) Diff
aar 7.23 kB 22.6 kB +15.3 kB (+212.2%)
apk (release) 2.08 MB 2.08 MB +3.67 kB (+0.2%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/IopnBF3HwG.html

rlazo · 2022-08-19T16:30:20Z

firebase-database/ktx/src/main/kotlin/com/google/firebase/database/ktx/Database.kt

+fun Query.snapshots() = callbackFlow<DataSnapshot> {
+    val listener = addValueEventListener(object : ValueEventListener {
+        override fun onDataChange(snapshot: DataSnapshot) {
+            trySend(snapshot)


For 7235473 we went with trySendBlocking is there any difference between cases?

When using trySendBlocking Android Studio shows me this warning:

I think it is a false positive in the linter.

That said, it looks like the events are dispatched on the UI thread, so it's not ok to directly trySendBlocking:

firebase-android-sdk/firebase-database/src/main/java/com/google/firebase/database/android/AndroidEventTarget.java

Line 25 in 58c1677

this.handler = new Handler(Looper.getMainLooper());

imo instead we can do the same thing we did for firestore, something like

Suggested change

trySend(snapshot)

someExecutor.execute(() -> trySendBlocking(snapshot));

@vkryachko Looks like firestore's background executor uses a ThreadPool with 4 threads:

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Executors.java

Lines 42 to 44 in d50a3d1

public static final Executor BACKGROUND_EXECUTOR =

new ThrottledForwardingExecutor(

ASYNC_THREAD_POOL_MAXIMUM_CONCURRENCY, AsyncTask.THREAD_POOL_EXECUTOR);

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Executors.java

Line 30 in d50a3d1

private static final int ASYNC_THREAD_POOL_MAXIMUM_CONCURRENCY = 4;

Should we also use a ThreadPool for RTDB?

Suggested change

trySend(snapshot)

Executors.newFixedThreadPool(nThreads = 4).execute {

trySendBlocking(ChildEvent.Added(snapshot, previousChildName))

}

@vkryachko You were right, it was a false-positive in the linter. After the AGP7 update, the lint warning went away.

But my question on what executor we'll want to use remains.

Not sure if that is safe, i.e. isn't in happening on the UI thread?

@maneesht what are your thoughts?

It seems like scheduleNow() delegates to DefaultRunLoop which uses a ScheduledThreadPoolExecutor , so it doesn't block the UI Thread.

firebase-android-sdk/firebase-database/src/main/java/com/google/firebase/database/core/utilities/DefaultRunLoop.java

Lines 65 to 69 in 6a566dd

public DefaultRunLoop() {

int threadsInPool = 1;

ThreadFactory threadFactory = new FirebaseThreadFactory();

executor =

new ScheduledThreadPoolExecutor(threadsInPool, threadFactory) {

My only concern with this is that we would end up blocking other operations within the Repo's actions. Maybe we should create a new executor solely for this? As RTDB only has one ATM

I thought this through a bit more, and let's go ahead and just use the default run loop. If we feel like there's a performance gain, we can always change this in a future patch.

Thanks for looking into it @maneesht !

…-sdk into rpf-add-rtdb-flow

firebase-database/ktx/src/main/kotlin/com/google/firebase/database/ktx/Database.kt

…-sdk into rpf-add-rtdb-flow

thatfiredev · 2022-09-23T09:57:28Z

@vkryachko I changed the Flows to be properties instead of functions. Please take a second look

thatfiredev added 2 commits August 19, 2022 16:31

add callbackFlow for RTDB's ValueEventListener

6d5d0df

add callbackFlow for RTDB's ChildEventListener

c696335

thatfiredev requested review from rlazo and vkryachko August 19, 2022 15:50

google-oss-bot added the size/L label Aug 19, 2022

explicitly declare type of callbackFlow

a91ad44

rlazo reviewed Aug 19, 2022

View reviewed changes

rlazo approved these changes Aug 19, 2022

View reviewed changes

vkryachko requested a review from maneesht August 19, 2022 18:43

thatfiredev added 3 commits September 15, 2022 17:11

Merge branch 'master' of https://github.com/firebase/firebase-android…

47a9970

…-sdk into rpf-add-rtdb-flow

trySend() --> trySendBlocking()

b9c04f0

refactor: delegate trySendBlocking to DefaultRunLoop

d800622

vkryachko reviewed Sep 21, 2022

View reviewed changes

firebase-database/ktx/src/main/kotlin/com/google/firebase/database/ktx/Database.kt Outdated Show resolved Hide resolved

vkryachko approved these changes Sep 21, 2022

View reviewed changes

thatfiredev added 5 commits September 22, 2022 15:07

add group to ktx.gradle

8c7aae1

Merge branch 'master' of https://github.com/firebase/firebase-android…

6f94eae

…-sdk into rpf-add-rtdb-flow

use project-wide coroutines version

e6e2c18

use properties instead of functions

527045b

update api.txt file

b2b7b7b

thatfiredev requested a review from vkryachko September 23, 2022 09:57

vkryachko approved these changes Sep 23, 2022

View reviewed changes

thatfiredev merged commit 3872957 into master Sep 23, 2022

thatfiredev deleted the rpf-add-rtdb-flow branch September 23, 2022 12:56

firebase locked and limited conversation to collaborators Oct 24, 2022

	trySend(snapshot)
	someExecutor.execute(() -> trySendBlocking(snapshot));

	public static final Executor BACKGROUND_EXECUTOR =
	new ThrottledForwardingExecutor(
	ASYNC_THREAD_POOL_MAXIMUM_CONCURRENCY, AsyncTask.THREAD_POOL_EXECUTOR);

-            trySend(snapshot)
+            Executors.newFixedThreadPool(nThreads = 4).execute {
+                trySendBlocking(ChildEvent.Added(snapshot, previousChildName))
+            }

	public DefaultRunLoop() {
	int threadsInPool = 1;
	ThreadFactory threadFactory = new FirebaseThreadFactory();
	executor =
	new ScheduledThreadPoolExecutor(threadsInPool, threadFactory) {

Conversation

thatfiredev commented Aug 19, 2022

Uh oh!

google-oss-bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-database-ktx

Test Logs

Uh oh!

github-actions bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-database-ktx

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thatfiredev commented Sep 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

google-oss-bot commented Aug 19, 2022 •

edited

Loading

Coverage Report ¹

`firebase-database-ktx`

github-actions bot commented Aug 19, 2022 •

edited

Loading

google-oss-bot commented Aug 19, 2022 •

edited

Loading

Size Report ¹

`firebase-database-ktx`