Platform Networking #3412
Platform Networking #3412
Conversation
# Conflicts: # Realm/Realm/Native/PrimitiveValue.cs
This reverts commit 410708d.
| catch (Exception e) | ||
| { | ||
| Logger.LogDefault(LogLevel.Error, $"Error occurred in SyncSocketProvider event loop {e.GetType().FullName}: {e.Message}"); | ||
| Logger.LogDefault(LogLevel.Trace, e.StackTrace); |
There was a problem hiding this comment.
Should we notify sync that we're terminating the work thread?
There was a problem hiding this comment.
There isn't an interface we can use to notify Sync about that. But now that you mention it the existing behavior on an exception on the sync worker thread is to bring down the process, so instead of swallowing and logging the exception that's what we should do here.
There was a problem hiding this comment.
A few more comments. I added a branch with suggestions here: #3421. Feel free to pick and choose those that make sense to you.
I also suggest that we use file-scoped namespaces for the new files - I didn't do it in my suggestions branch because then it would reorder too much stuff making it difficult to review the actual suggestions.
| await _webSocket.ConnectAsync(_uri, _cts.Token); | ||
| await _workQueue.WriteAsync(new WebSocketConnectedWork(_webSocket.SubProtocol, _observer, _cts.Token)); | ||
| } | ||
| catch (WebSocketException e) |
There was a problem hiding this comment.
Should we send a websocket closed work in case of a non-websocket error - e.g. I can imagine we can hit an out of memory or similar system exception that would prevent us from connecting but Sync will be waiting forever for the connection.
Realm/Realm/Sync/App.cs
Outdated
| if (Environment.GetEnvironmentVariable("REALM_DOTNET_USE_LEGACY_WEBSOCKET") == null) | ||
| { | ||
| var provider = new SyncSocketProvider(config.OnSyncWebSocketConnection); | ||
| nativeConfig.managed_websocket_provider = GCHandle.ToIntPtr(GCHandle.Alloc(provider)); | ||
| } |
There was a problem hiding this comment.
For the initial rollout, I suggest that we use OnSyncWebsocketConnection and tell customers that want to opt in to set the callback to a non-null value.
fealebenpae
force-pushed
the
yg/websocket
branch
from
e77b779
to
8c532d4
Compare
Pull Request Test Coverage Report for Build 5869588278
💛 - Coveralls |
| Logger.LogDefault(LogLevel.Trace, e.StackTrace); | ||
| } | ||
|
|
||
| // TODO: The documentation for WebSocketObserver::async_write_binary() says the handler should be called with RuntimeError in case of errors |
There was a problem hiding this comment.
Can we ping someone on the sync team to get an answer here?
| Logger.LogDefault(LogLevel.Trace, e.StackTrace); | ||
| } | ||
|
|
||
| throw; |
There was a problem hiding this comment.
I don't think throwing in a background thread will bring down the process - you may get a warning or something, but in release, I think it's just ignored on most platforms.
Description
Add option to use ClientWebSocket instead of the built-in WebSocket client for Sync.
Fixes #2911
TODO