Add support for automatic resetting incremental backoff when network status changes #1491
Conversation
…changes are detected
Fix buggy teardown of custom headers test
|
Finally ready for review. For some reason, this PR exposed some race conditions in our test setup caused by us not shutting down all test apps correctly. So a long list of changes here are just associated with fixing this (Basically all apps now have a unique ID that will make it easier to find and fix threading test errors in the future) |
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/sync/Sync.kt
Outdated
Show resolved
Hide resolved
| import io.realm.kotlin.internal.interop.SynchronizableObject | ||
|
|
||
| // Register a system specific network listener (if supported) | ||
| internal expect fun registerSystemNetworkObserver() |
There was a problem hiding this comment.
I understand the idea, but since all implementations are no-ops, does it then make sense to have it at all 🤔
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/AppImpl.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/sync/Sync.kt
Show resolved
Hide resolved
| fun isConnected(cm: ConnectivityManager?): Boolean { | ||
| return cm?.let { | ||
| val networkInfo: NetworkInfo? = cm.activeNetworkInfo | ||
| networkInfo != null && networkInfo.isConnectedOrConnecting || isEmulator() |
There was a problem hiding this comment.
Why are emulator always connected?
There was a problem hiding this comment.
No idea 😅 ... I pulled this code from somewhere else and has never really questioned it 🙈
| @@ -49,7 +49,7 @@ class ApiKeyAuthTests { | |||
|
|
|||
| @BeforeTest | |||
| fun setup() { | |||
| app = TestApp(appName = TEST_APP_PARTITION) | |||
| app = TestApp(this::class.simpleName, appName = TEST_APP_PARTITION) | |||
There was a problem hiding this comment.
I guess not for this PR, but would be awfully nice if we could just inject the test name from a rule or something like that. This seems like a really annoying thing to have to always write ... but I guess less annoying than trying to debug without it 😅
packages/test-sync/src/commonMain/kotlin/io/realm/kotlin/test/mongodb/TestApp.kt
Show resolved
Hide resolved
...ages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/CredentialsTests.kt
Show resolved
Hide resolved
packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/SyncClientTests.kt
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/sync/Sync.kt
Show resolved
Hide resolved
| @@ -32,7 +32,7 @@ public data class RealmInstantImpl(override val seconds: Long, override val nano | |||
| } | |||
| } | |||
|
|
|||
| internal fun RealmInstant.toDuration(): Duration { | |||
| public fun RealmInstant.toDuration(): Duration { | |||
There was a problem hiding this comment.
Not for this PR, but we could consider to make this a public-public extension.
| } | ||
| if (connectionAvailable && (lastOnlineStateReportedMs == null || nowMs - lastOnlineStateReportedMs!! > reconnectThresholdMs) | ||
| val now: Duration = RealmInstant.now().toDuration() | ||
| if (connectionAvailable && (lastOnlineStateReported == null || now.minus(lastOnlineStateReported!!) > reconnectThreshold) |
There was a problem hiding this comment.
🎉 ... I guess you could make it more Kotlin idiomatic with something like
if (connectionAvailable && (lastOnlineStateReported?.let { now - it > reconnectThreshold } != false))
but don't mind keeping it as is.
Closes #1479
This PR exposes a number of methods relevant to controlling the network connection and session lifecycle. It does this through a new
Syncclass that is exposed throughApp.Sync, similar to Realm Java.Before this PR, reconnecting was handled purely by Core, which used incremental backoff. In the worst case, this meant that the Sync connection was not established until 5 minutes after the device was online again (the limit in Core)
With this PR a new
Sync.reconnect()method is added that makes it possible to trigger a manual reconnect if users have better insight into when the network condition changed, e.g. through theConnectivityManageron Android.It also adds a best-effort implementation for doing this automatically: https://github.com/realm/realm-kotlin/pull/1491/files#diff-835b655f827ffc568b4fd6c04722b17faf9ec56fe73289379bfdad6723d5a00bR79.
Note, that automatic reconnect detection is only available on Android, iOS, and macOS. JVM has no APIs we can hook into.
TODO