-
Notifications
You must be signed in to change notification settings - Fork 61
[Fix] Unhandled exception on reconnect #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| * Triggered whenever a status update has been attempted to be made or | ||
| * refreshed. | ||
| */ | ||
| statusUpdated?: ((statusUpdate: SyncStatusOptions) => void) | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we need both of these. Is it not possible to get the events you need with only statusChanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current reason for adding this is to add a bit more synchronisation to the powersync.connect method. Currently it's an awaitable async method, but it's also a bit of a "fire and forget" method which doesn't actually resolve once the client is connected.
The powersync.connect method does automatically retry on failed connection attempts, so it does not throw if the initial connection fails. We could add that ability in future, but it seems to be a breaking change for existing code which would potentially throw unhandled exceptions. Essentially now the powersync.connect method will resolve once the connection is either established or if the first attempt failed - the user can still check the actual status afterwards. This is not ideal, but it does add some more function to the current implementation.
If the connection fails to connect then there is a status update for connected: false which is used to resolve the connect call. The statusChanged listener only fires if the status actually changed from the previous status, this could not be used if the client was previously disconnected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, I noticed now in development testing I have resulted in not included the await on the PowerSyncDatabase client level. Added that now.
This PR addresses some issues identified in the recent
0.3.0web SDK release.Reconnect
Aborting requests
This fixes a regression from #101 which caused an unhandled exception to be thrown in the shared sync worker when cancelling active requests to
sync/stream. When closing the connection via the abort signal the existing stream would throw a unhandled exception "DOMException: BodyStreamBuffer was aborted".The implementation here handles abort controller events on the fetch request if the response stream has not yet been resolved. If the response stream has been resolved and is actively in use, then the signal will be used to close the stream instead of aborting the already resolved fetch request.
All abort requests now also contain a reason in the form of an
AbortOperationthis prevents error logs for abort operations.This method of aborting is currently duplicated in the React Native and Web SDKs. A future websocket PR will cleanup this duplication in the common-SDK package.
Multiple calls to
.connectThis PR also adds
navigatorlocks on the SharedSyncImplementation'sconnectanddisconnectmethods. This effectively queues attempts to connect/reconnect. Previously callingpowersync.connect(...)multiple times simultaneously could cause race conditions where the multiple connect/disconnect operations would interfere with each other.Testing:
This work was tested in our demo applications by adding multiple calls to
powersync.connect. The dev tools verify that old stream connections are correctly closed.Broadcast logger improvements
This adds more sanitation to broadcast logging and also prevents any exceptions from the logging action (e.g. due to data cloning errors) to propagate as external errors.
This PR also adds the ability to opt-out of broadcasted logs. Some clients have reported strange comlink proxy errors for the broadcast logger. These errors are currently not reproducible in our demo applications. Broadcast logs are enabled by default, but can be disabled as a stop gap for now.