Platform networking #1528

nhachicha · 2023-09-26T15:56:13Z

Add the option to use Ktor as the Websocket client instead of the built-in C++ WebSocket client for Sync.

TODO

Changelog entry
Switch to a dedicated single-threaded worker?

Waiting

Core to merge Added sync socket result enum for sync socket callback handlers in C API realm-core#7015 to update commit hash in the changelog

…Observer to detach it

cmelchior

Just dropping an initial review of auxiliary files before diving into the WebSocket code.

Mostly clarifying questions, but also looks like we should update Core on main so you can merge that into this branch.

cmelchior · 2023-10-11T14:46:10Z

CHANGELOG.md

@@ -16,6 +16,7 @@
 * [Sync] If calling a function on App Services that resulted in a redirect, it would only redirect for 
 GET requests. (Issue [#1517](https://github.com/realm/realm-kotlin/pull/1517))
 * [Sync] If calling a function on App Services that resulted in a redirect, it would only redirect for GET requests. (Issue [#1517](https://github.com/realm/realm-kotlin/pull/1517))
+* [Sync] Added option to use managed WebSockets via Ktor instead of Realm's built-in WebSocket client for Sync traffic. Managed WebSockets offer improved support for proxies and firewalls that require authentication. This feature is currently opt-in and can be enabled by using `AppConfiguration.usePlatformNetworking()`. Managed WebSockets will become the default in a future version. (PR [#1528](https://github.com/realm/realm-kotlin/pull/1528)). Currently only JVM and Android platforms are supported.


Great description 💯

buildSrc/src/main/kotlin/Config.kt

cmelchior · 2023-10-11T14:47:58Z

Jenkinsfile

@@ -188,7 +188,7 @@ pipeline {
                                    "integrationtest",
                                    {
                                        forwardAdbPorts()
-                                        testAndCollect("packages", "cleanAllTests -PincludeSdkModules=false connectedAndroidTest")
+                                        testAndCollect("packages", "cleanAllTests -PREALM_USE_PLATFORM_NETWORKING=true -PincludeSdkModules=false connectedAndroidTest")


Is it correct that we no longer run full unit tests on the old implementation?

Yes, this is running with the new implementation for Android/JVM, we can duplicate the job to run also on legacy impl

cmelchior · 2023-10-11T14:48:47Z

packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/UserTests.kt

-        // AuthenticationProvider is not removed once user is logged out
-        assertEquals(AuthenticationProvider.EMAIL_PASSWORD, emailUser.provider)
-    }
+//    @Test


No, this is part of updating Core realm_user_get_auth_provider is no longer available

cmelchior · 2023-10-11T14:49:34Z

...ages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/SyncedRealmTests.kt

@@ -1260,7 +1260,8 @@ class SyncedRealmTests {
            }
            assertTrue(customLogger.logs.isNotEmpty())
            assertTrue(customLogger.logs.any { it.contains("Connection[1]: Negotiated protocol version:") }, "Missing Connection[1]")
-            assertTrue(customLogger.logs.any { it.contains("MyCustomApp/1.0.0") }, "Missing MyCustomApp/1.0.0")
+            // user_agent is not populated in the new socket provider


Is this a bug or intended?

This is part of the new Core update see https://mongodb.slack.com/archives/C010R3CMP3N/p1694531686031569?thread_ts=1694521809.008969&cid=C010R3CMP3N

cmelchior · 2023-10-11T14:59:07Z

...es/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/sync/ProtocolErrorCode.kt

+    }
+}
+
+expect enum class WebsocketCallbackResult : CodeDescription {


Tests for this mapping should be added to SyncEnumTests

cmelchior · 2023-10-11T14:59:50Z

packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

@@ -1115,7 +1118,8 @@ actual object RealmInterop {
    }

    actual fun realm_user_get_auth_provider(user: RealmUserPointer): AuthProvider {
-        return AuthProvider.of(realmc.realm_user_get_auth_provider(user.cptr()))
+        TODO("No longer valid")
+//        return AuthProvider.of(realmc.realm_user_get_auth_provider(user.cptr()))


We should probably upgrade Core in a separate PR from this one...there is quite a few things in here only related to that.

cmelchior · 2023-10-11T15:05:04Z

packages/library-sync/src/jvm/kotlin/io/realm/kotlin/mongodb/internal/HttpClientCache.kt

-                retryOnConnectionFailure(true)
-            }
-        }
+    // Revert to OkHttp when https://youtrack.jetbrains.com/issue/KTOR-6266 is fixed


We should probably mention this in the internal changelog

cmelchior · 2023-10-11T15:08:08Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/HttpClientCache.kt

@@ -48,4 +48,5 @@ internal expect class HttpClientCache(timeoutMs: Long, customLogger: Logger? = n
    fun close() // Close any resources stored in the cache.
 }

+// TODO use a la private val clientCache: HttpClientCache = HttpClientCache(timeoutMs, logger)


Not 100% sure what this means?

cmelchior · 2023-10-11T15:12:24Z

...ges/library-sync/src/nativeDarwin/kotlin/io/realm/kotlin/mongodb/internal/HttpClientCache.kt

@@ -19,5 +20,8 @@ internal actual class HttpClientCache actual constructor(timeoutMs: Long, custom
 }

 public actual fun createPlatformClient(block: HttpClientConfig<*>.() -> Unit): HttpClient {
-    return HttpClient(Darwin, block)
+    return HttpClient(Darwin) {
+        install(WebSockets)


Why are we installing it here if Darwin isn't supported yet?

Also it seems to already being installed here: https://github.com/realm/realm-kotlin/pull/1528/files#diff-2270f40a8e4fabaacf4f2791bbe75dee7428db4c09f84fc606ab90aaa99a78e1R277

- Closing JVM scheduler in dtors

- Update to CMake 3.27.7 - Enabling sync tests for Darwin

- Fix merge error

clementetb

Added some minor comments I am still reviewing the Transport implementation

clementetb · 2023-11-10T12:54:32Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt

+         * Platform Networking offer improved support for proxies and firewalls that require authentication,
+         * instead of Realm's built-in WebSocket client for Sync traffic. This will become the default in a future version.
+         */
+        public fun usePlatformNetworking(enable: Boolean = true): Builder =


Should we annotate it as experimental? It will become a breaking change when it becomes stable and we remove it.

Good idea, I think though we use experimental annotations like ExperimentalAsymmetricSyncApi & ExperimentalFlexibleSyncApi to advertise the ability to change the API without necessary relying on pruning / semantic versioning, which is not the case here, the API will be removed on a next major not evolve over time...

packages/test-sync/build.gradle.kts

...s/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/sync/WebSocketTransport.kt

clementetb · 2023-11-10T14:21:21Z

.../cinterop/src/nativeDarwin/kotlin/io/realm/kotlin/internal/interop/sync/ProtocolErrorCode.kt

@@ -132,8 +132,10 @@ actual enum class WebsocketErrorCode(

    override val nativeValue: Int = errorCode.value.toInt()

+    val asNativeEnum: realm_web_socket_errno = errorCode


Couldn't just elevate errorCode to be a class property?

it is not shared/used for WebsocketErrorCode JVM actual implementation

clementetb · 2023-11-10T14:25:26Z

packages/jni-swig-stub/src/main/jni/realm_api_helpers.h

@@ -137,4 +137,16 @@ realm_sync_thread_error(realm_userdata_t userdata, const char* error);
 realm_scheduler_t*
 realm_create_generic_scheduler();

+realm_sync_socket_t* realm_sync_websocket_new(int64_t sync_client_config_ptr, jobject websocket_transport);


how come pointers are int64_t and not void*?

We're already using int64_t for pointers. What would be the difference? anyway it will hold the value of jlong which should be fine (even on 32 bits arch I believe)

clementetb · 2023-11-10T14:27:32Z

...ibrary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/KtorWebSocketTransport.kt

@@ -0,0 +1,322 @@
+package io.realm.kotlin.mongodb.internal


clementetb · 2023-11-10T14:32:22Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt

@@ -393,6 +406,14 @@ public interface AppConfiguration {
                    )
                }

+            val websocketTransport: ((DispatcherHolder) -> WebSocketTransport)? =
+                if (usePlatformNetworking) { dispatcherHolder ->
+                    websocketTransport ?: KtorWebSocketTransport(


Are there any logs we like to forward?

clementetb · 2023-11-10T14:33:12Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt

+            val websocketTransport: ((DispatcherHolder) -> WebSocketTransport)? =
+                if (usePlatformNetworking) { dispatcherHolder ->
+                    websocketTransport ?: KtorWebSocketTransport(
+                        timeoutMs = 60000,


We might like to update #408 to add another config option for this timeout.

clementetb

The web socket logic might need some refactoring to ease reasoning on it. Logic could even be reduced by using scope jobs.

Otherwise, I am happy when CI is happy, it is a experimental feature 🧪

clementetb · 2023-11-13T12:20:50Z

...ibrary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/KtorWebSocketTransport.kt

+        }
+    }
+
+    override fun connect(


I agree with Claus, the connect method is way too long with many implementation details.

It would be easier to reason if, at least, the WebSocketClient class was outside of the connect method, that way we would have a clear distinction between the Core API and the web sockets API.

clementetb · 2023-11-13T12:30:59Z

...ibrary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/KtorWebSocketTransport.kt

+
+                            // Writing messages to WebSocket
+                            scope.launch {
+                                writeChannel.consumeEach {


Is there any reason we are not using jobs instead of this channel write? We could benefit of the existing scope job handling to do this instead of iterating over the channel.

On each core-post we would post a job to the scope to the actual write.

clementetb · 2023-11-13T12:35:32Z

...ibrary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/KtorWebSocketTransport.kt

+                    session.cancel() // Terminate the WebSocket session, connect needs to be called again.
+                }
+                // Collect unprocessed writes and cancel them (mainly to avoid leaking the FunctionHandler).
+                while (true) {


Following up, by using the scope-job way, here by canceling the scope we would cancel any pending writes. The runcallback could be invoked automatically if these scope-jobs are configured with invokeOnCompletion.

…id. KTOR doesn't expose Close Frames which are necessary to rely any error code to Core (invalid token for example) CIO client also has issues with Android doze https://youtrack.jetbrains.com/issue/KTOR-5993

- Fixing CMake conf

rorbech

Order of magnitudes easier to understand and follow the logic 💪😎 Could use some comments on the classes from WebSocketTransport.kt to give an faster entry point, but quite easy to follow when you dive into the implementation.

It feels a bit odd to schedule all websockets on the same thread, but can't really judge if I feel it is worth reworking at the moment 🤔

rorbech · 2023-12-08T08:26:49Z

...ages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/SyncedRealmTests.kt

@@ -943,10 +943,12 @@ class SyncedRealmTests {
            val (email1, password1) = randomEmail() to "password1234"
            val user = flexApp.createUserAndLogIn(email1, password1)
            val localConfig = createWriteCopyLocalConfig("local.realm")
-            val syncConfig = createPartitionSyncConfig(
+            val syncConfig = createFlexibleSyncConfig(


🙈 ... 😅

...s/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/sync/WebSocketTransport.kt

rorbech · 2023-12-08T08:53:49Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt

@@ -373,6 +377,17 @@ public interface AppConfiguration {
            return this
        }

+        /**


Is it obvious what Platform networking is actually referring to? Should be a bit more explicit as in the CHANGELOG?

rorbech · 2023-12-08T09:03:15Z

...brary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/RealmWebSocketTransport.kt

+    override fun post(handlerCallback: RealmWebsocketHandlerCallbackPointer) {
+        scope.launch {
+            (this as Job).invokeOnCompletion { completionHandler: Throwable? ->
+                // Only run the callback if it was not cancelled in the meantime


Nit pick. The comment seems a bit off? You are actually calling runCallback in each case. Also feels a bit weird that cancelled = true when completionHandler == null. Maybe just a matter of naming 🤪

Yes the wording is weird. The point is to "run successfully" the callback if there was no Cancellation error. cancelled = true should be set if coroutine was cancelled (i.e completionHandler contains an error or a CancellationException)

rorbech · 2023-12-08T09:35:09Z

...brary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/RealmWebSocketTransport.kt

+    public val timeoutMs: Long
+) : WebSocketTransport {
+    // we need a single thread dispatcher to act like an event loop
+    private val dispatcherHolder: DispatcherHolder = CoroutineDispatcherFactory.managed("RealmWebsocketTransport").create()


I feel that it is wrong level of parallelism here. Seems weird that we have the options to selection multiplexing or not, but multiple websocket will be operation on the same thread. I don't know if this would have any actual effect though 🤔

Having a dispatcher per websocket will only benefit if we have a full-duplex apps running in parallel which is a rare use case. Most use cases are the same app sharing the same transport, which is more inlined with multiplexing becoming the default...
We can revisit these approximations in case of a performance issue.

packages/library-sync/src/jvm/kotlin/io/realm/kotlin/mongodb/internal/OkHttpWebsocketClient.kt

cmelchior

Great 🚀 ... I only have a few minor comments/questions.

CHANGELOG.md

...s/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/sync/WebSocketTransport.kt

cmelchior · 2023-12-11T08:15:04Z

packages/test-sync/src/jvmTest/kotlin/io/realm/kotlin/test/mongodb/jvm/RealmTests.kt

@@ -41,7 +43,11 @@ class RealmTests {
        val app = TestApp("cleanupAllRealmThreadsOnClose")
        val user = app.login(Credentials.anonymous())
        val configuration = SyncConfiguration.create(user, TestHelper.randomPartitionValue(), setOf(ParentPk::class, ChildPk::class))
-        Realm.open(configuration).close()
+        Realm.open(configuration).use {
+            // we make sure Schema is exchanged correctly


Did you encounter some kind of error if this didn't happen?

In the beginning... although it was using the old Ktor implementation, I'll revert

packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp

cmelchior · 2023-12-11T08:50:04Z

...ibrary-sync/src/nativeDarwin/kotlin/io/realm/kotlin/mongodb/internal/NetworkStateObserver.kt

+    isSsl: Boolean,
+    supportedSyncProtocols: String,
+    transport: RealmWebSocketTransport
+): WebSocketClient = TODO()


Maybe create an issue specifically tracking support for this to Darwin? And then link it here

...brary-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/RealmWebSocketTransport.kt

cmelchior · 2023-12-11T09:15:53Z

packages/library-sync/src/jvm/kotlin/io/realm/kotlin/mongodb/internal/OkHttpWebsocketClient.kt

+    private val protocolSelectionHeader = "Sec-WebSocket-Protocol"
+
+    init {
+        val websocketURL = "${if (isSsl) "wss" else "ws"}://$address:$port$path"


I thought this URL was set by the Location request in Core? Should we built it up ourselves?

Yes, Core provides this endpoint.

packages/library-sync/src/jvm/kotlin/io/realm/kotlin/mongodb/internal/OkHttpWebsocketClient.kt

- Changelog

nhachicha added 3 commits September 25, 2023 13:26

Using platform networking for Sync Websocket

88c9565

Merge remote-tracking branch 'origin/main' into nh/platform_networking

49c757d

Disabling removed C-API method

2470859

cla-bot bot added the cla: yes label Sep 26, 2023

github-actions bot assigned nhachicha Sep 26, 2023

nhachicha added 10 commits October 5, 2023 10:22

Completing appropriately the Sync callbacks

824e049

Fixes

e521d62

Merge remote-tracking branch 'origin/main' into nh/platform_networking

455ab2e

Fixing Swig

698e86a

Fixing indentation

d64b0b2

Fixing test

16b62ab

Adding a flag to run new platform selectively on CI

3505926

Fixing tests/cleanup

aa38a74

Using daemon thread for sync client while Core add support to Thread …

01e6d62

…Observer to detach it

Proguard fix

c92871c

nhachicha marked this pull request as ready for review October 11, 2023 11:34

nhachicha requested review from cmelchior, clementetb and rorbech and removed request for cmelchior October 11, 2023 11:34

Update changelog

2c68075

cmelchior reviewed Oct 11, 2023

View reviewed changes

nhachicha and others added 7 commits October 12, 2023 11:48

Initial PR feedback

bb4105b

Fix indentation

4de15a9

Bump Core

da49df3

Fixing renamed types

7133797

Bump core to version 13.23.0

5098395

Remove workaround for realm file name

c0180c5

Update metadata schema version

5234ba4

nhachicha added 9 commits October 19, 2023 16:19

Fixing tests

dd37a22

Fixing test

073e4be

- Update Core

0e41956

- Closing JVM scheduler in dtors

unused import

0105a73

Reverting changes

aecef93

- Revert engines to OkHttp for JVM and Android and CIO for Darwin

1120c1b

- Update to CMake 3.27.7 - Enabling sync tests for Darwin

Merge remote-tracking branch 'origin/main' into nh/platform_networking

ea50805

Disabling Darwin Sync tests

fed3971

- Update Core commit

fdb717a

- Fix merge error

clementetb reviewed Nov 10, 2023

View reviewed changes

clementetb reviewed Nov 13, 2023

View reviewed changes

nhachicha added 8 commits November 25, 2023 18:24

Reworked the Ktor Websocket Trasnport to use OkHttp for JVM and Andro…

50639db

…id. KTOR doesn't expose Close Frames which are necessary to rely any error code to Core (invalid token for example) CIO client also has issues with Android doze https://youtrack.jetbrains.com/issue/KTOR-5993

- Cancelled callback when Websocket is closed

042eb8a

- Fixing CMake conf

Merge remote-tracking branch 'origin/main' into nh/platform_networking

ccda0ec

fixing compile error

71359d3

fixing test

a55e9ed

PR feedback

96be9c1

Merge remote-tracking branch 'origin/main' into nh/platform_networking

486686c

Added removed configuration

0ae12f8

nhachicha requested review from cmelchior, rorbech and clementetb December 6, 2023 11:35

rorbech approved these changes Dec 8, 2023

View reviewed changes

cmelchior reviewed Dec 11, 2023

View reviewed changes

nhachicha and others added 3 commits December 11, 2023 14:36

PR feedback

d491fee

Merge branch 'main' into nh/platform_networking

e423e3e

- Unused imports

0c8dca8

- Changelog

nhachicha merged commit 445a6a9 into main Dec 12, 2023
2 checks passed

nhachicha deleted the nh/platform_networking branch December 12, 2023 13:43

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

		@@ -132,8 +132,10 @@ actual enum class WebsocketErrorCode(

		override val nativeValue: Int = errorCode.value.toInt()

		val asNativeEnum: realm_web_socket_errno = errorCode

Platform networking #1528

Platform networking #1528

Conversation

nhachicha commented Sep 26, 2023 • edited Loading

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementetb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhachicha Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementetb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rorbech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhachicha commented Sep 26, 2023 •

edited

Loading

nhachicha Dec 6, 2023 •

edited

Loading