Skip to content
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

fix: Better stream and access token management #1019

Merged
merged 17 commits into from
Sep 26, 2024
Merged

fix: Better stream and access token management #1019

merged 17 commits into from
Sep 26, 2024

Conversation

Vinzent03
Copy link
Collaborator

@Vinzent03 Vinzent03 commented Aug 28, 2024

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

The behavior of the app in background depends on the platform, but by always stopping the access token refreshing, the realtime connection may keep an invalid access token and therefore the connection get closed by server. When the realtime connection gets interrupted, it gets automatically reconnected as long as it never sent some expired access token.

What is the new behavior?

When a realtime channel is open, the access token keeps refreshing while the app is in background.

In addition, I did more management to keep realtiem and postgrest in sync for the stream method. If the postgrest request fails, I unsubscribe the realtime connection and close the stream as I see no benefit in leaving it open, as it would just be a realtime connection by then. The dev using the stream method should handle such cases. One could also argue about doing a few retries as well, though.
When the realtime connection gets a channel error, but gets resubscribed, I reload the postgrest data to ensure the data is up to date and didn't miss any updates due to the interrupted realtime connection.

Additional context

#1012 #705 #579

@Vinzent03 Vinzent03 changed the title fix: realtime issues fix: better stream and access token management Sep 2, 2024
@Vinzent03 Vinzent03 changed the title fix: better stream and access token management fix: Better stream and access token management Sep 2, 2024
@coolusaHD
Copy link

Hey @Vinzent03

This PR looks nice and maybe could also be the solution of our problem #674 .
If you can reply here or in the issue thread we would love to test the update when you finished your work ✌️

Thanks 🚀

@Vinzent03 Vinzent03 marked this pull request as ready for review September 9, 2024 16:36
@Vinzent03 Vinzent03 requested a review from dshukertjr September 9, 2024 16:36
Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. This looks awesome. I'm a bit swamped today and maybe tomorrow, but let me take a look whenever I can.

@dshukertjr
Copy link
Member

dshukertjr commented Sep 10, 2024

@Vinzent03 BTW, have you actually run this code on iOS and Android? I tried to fix some of the realtime issues and found that some behaviors differ on the web sockets between those two OSs. I don't remember what exactly those differences were, but wanted to ask if you have tested on actual apps.

@Vinzent03
Copy link
Collaborator Author

I have no way to test this on any Apple device. I tested this on my android device and as a linux application. The stopping of refreshing the access token was especially an issue in the linux application, because the realtime connection keeps being open, when the app is in background, because there is no throttling after some time.

@maxfornacon

This comment was marked as off-topic.

@coolusaHD

This comment was marked as off-topic.

@maxfornacon

This comment was marked as off-topic.

@maxfornacon
Copy link

I started the Application on macOS, closed my MacBook and went home. When I opened it at home I saw:

AuthException(message: ClientException with SocketException: Failed host lookup: 'xxx.supabase.co' (OS Error: nodename nor servname provided, or not known, errno = 8), uri=https:/xxx.supabase.co/auth/v1/token?grant_type=refresh_token, statusCode: null, errorCode: null)
flutter: #0      GotrueFetch._handleRequest (package:gotrue/src/fetch.dart:179:7)
flutter: <asynchronous suspension>
flutter: #1      GotrueFetch.request (package:gotrue/src/fetch.dart:130:12)
flutter: <asynchronous suspension>
flutter: #2      GoTrueClient._refreshAccessToken.<anonymous closure> (package:gotrue/src/gotrue_client.dart:1052:26)
flutter: <asynchronous suspension>
flutter: #3      RetryOptions.retry (package:retry/retry.dart:131:16)
flutter: <asynchronous suspension>
flutter: #4      GoTrueClient._refreshAccessToken (package:gotrue/src/gotrue_client.dart:1044:12)
flutter: <asynchronous suspension>
flutter: #5      GoTrueClient._callRefreshToken (package:gotrue/src/gotrue_client.dart:1150:20)
flutter: <asynchronous suspension>
flutter: #6      GoTrueClient._autoRefreshTokenTick (package:gotrue/src/gotrue_client.dart:1032:9)

This indicates a loss of internet connection, as expected.

Also the StreamBuilder printed out RealtimeSubscribeException(status: RealtimeSubscribeStatus.channelError, details: null).

This is an improvement!

If it could auto reconnect would be even better. But that's something I could work with.

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core issue with the realtime currently is the fact that the SDK isn't able to reconnect in the following scenario:

  1. app is connected to realtime
  2. realtime is disconnected (bad network or app goes to the background) long enough for the JWT to expire
  3. app comes back online

At step 3 above, the connection should be restored, and that is the behavior of the js SDK. I have spend some time trying to fix the issue, but have had no success on this SDK, but if we could fix that, all the other issues should be fixed fairly straight forward.

packages/supabase_flutter/lib/src/supabase_auth.dart Outdated Show resolved Hide resolved
@Vinzent03 Vinzent03 force-pushed the fix/realtime branch 2 times, most recently from 980f368 to 7dc9dcb Compare September 16, 2024 20:41
@Vinzent03
Copy link
Collaborator Author

Vinzent03 commented Sep 18, 2024

I've done now quite more changes.

  • I've undone fix: Remove error parameter on _triggerChanError #637 by again emitting the exact channel error, so that one can handle SocketExceptions and so on explicitly
  • The errors received from onError aren't stringified anymore.
  • I had to make connect and disconnect async, because the previous code was wrong. We have to wait for the conn.ready future to be completed before using the sink.
  • Added a new SocketStates.disconnecting since disconnecting had to be async as well, and emitting a SocketStates.disconnected, while it's not actually disconnected is wrong.
  • Removed AppLifecycleState.inactive as state to trigger stopping the refresh token and disconnecting realtime, because from reading the documentation this is called often and does not result in stopping the programm code
  • When the app goes to actually background I now disconnect the realtime client. When resuming the work is more complicated. When quickly switching between fore and background and having a bad internet connection, the disconnection may still be running, which needed some extra care, but should be well documented in the code comments.
  • I needed the method RealtimeChannel.forceRejoin(), because the channel is still marked as joined, which results in unsubscribing itself in leaveOpenTopic(), which resulted in only closed channels.

Your comment should be working now as expected. I've been testing this on android, linux and web.
@maxfornacon Would love if you could give this version another try.

@themightychris
Copy link

The core issue with the realtime currently is the fact that the SDK isn't able to reconnect in the following scenario:

  1. app is connected to realtime
  2. realtime is disconnected (bad network or app goes to the background) long enough for the JWT to expire
  3. app comes back online

At step 3 above, the connection should be restored, and that is the behavior of the js SDK. I have spend some time trying to fix the issue, but have had no success on this SDK, but if we could fix that, all the other issues should be fixed fairly straight forward.

I consistently see the SDK fail to reconnect when the above happens well within the JWT's expiration time, is there another explanation for that addressed here?

@maxfornacon
Copy link

Thanks @Vinzent03 for your amazing work.

I made a few tests where I had the application open for hours during the day or over night. The stream still received the changes after that, every time. This is huge!

Nevertheless, I got quite a lot of Exceptions in the log.

Mostly:

Unhandled Exception: RealtimeSubscribeException(status: RealtimeSubscribeStatus.channelError, details: RealtimeCloseEvent(code: 1002, reason: null))
#0      SupabaseStreamBuilder._addException (package:supabase/src/supabase_stream_builder.dart:331:67)
#1      SupabaseStreamBuilder._getStreamData.<anonymous closure> (package:supabase/src/supabase_stream_builder.dart:229:11)
#2      RealtimeChannel.subscribe.<anonymous closure> (package:realtime_client/src/realtime_channel.dart:127:39)
#3      RealtimeChannel._onError.<anonymous closure> (package:realtime_client/src/realtime_channel.dart:265:36)
#4      RealtimeChannel.trigger (package:realtime_client/src/realtime_channel.dart:718:22)
#5      RealtimeClient._triggerChanError (package:realtime_client/src/realtime_client.dart:451:15)
#6      RealtimeClient._onConnClose (package:realtime_client/src/realtime_client.dart:432:7)
#7      RealtimeClient.connect.<anonymous closure> (package:realtime_client/src/realtime_client.dart:190:11)
Unhandled Exception: RealtimeSubscribeException(status: RealtimeSubscribeStatus.channelError, details: RealtimeCloseEvent(code: 1005, reason: ))

What I haven't tested yet is what happens when changes occur while the application is paused. Whether they are reliably loaded when resuming.

@Vinzent03
Copy link
Collaborator Author

@themightychris Are you sure you've used the latest state of this pr? For me and looks like for @maxfornacon it works fine.

@maxfornacon These errors are actually expected, because the websocket connection fails when loosing internet or the app is in background. I think it's better to add them to the .stream() stream, so you can react when the connection is interrupted.

@themightychris
Copy link

themightychris commented Sep 23, 2024

@Vinzent03 I have not tested this PR in a real environment yet, I was just responding to the description of the problem that seemed to suggest JWT expiration being the root of it, which would make #674 a separate issue—I came to this PR from experiencing #674 looking if the "better stream management" end of this work could solve for that too

@Vinzent03
Copy link
Collaborator Author

@themightychris Depends on the exact issue you are facing, but hopefully this pr should address many cases, and I think #674 as well.

@maxfornacon
Copy link

maxfornacon commented Sep 23, 2024

For me and looks like for @maxfornacon it works fine.

It works for a long time, but eventually the stream gets closed.
I receive those errors from snapshot.error of a StreamBuilder:

RealtimeSubscribeException(status: RealtimeSubscribeStatus.channelError, details: RealtimeCloseEvent(code: 1000, reason: heartbeat timeout))

and

RealtimeSubscribeException(status: RealtimeSubscribeStatus.timedOut, details: null)

These errors are actually expected, because the websocket connection fails when loosing internet or the app is in background. I think it's better to add them to the .stream() stream, so you can react when the connection is interrupted.

Yes, but there are a lot of the exceptions, but the stream seams to work fine despite of them. Wouldn't handling them cause more harm than good?

Handling the above mentioned exceptions would be fine, because then the stream is really closed and it makes sense to show it to the user to manually reload or implement some kind of auto refresh.

@Vinzent03
Copy link
Collaborator Author

The heartbeat exception is thrown from the client side, because the server doesn't answer or similar. But this shouldn't cause the stream to be closed. Adding exceptions to the stream doesn't mean it's closed. The connection of the realtime client may be interrupted, but the stream shouldn't be closed yet. You can determine when a stream is closed by the onDone callback in theStream.listen().
From the package side, there should be no uncaught exceptions. They should all be caught and then added to the stream. You are then supposed to catch them by adding the onError and onDone callback when using theStream.listen().
https://api.flutter.dev/flutter/dart-async/Stream/listen.html

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a little comment, but looks awesome!

packages/realtime_client/lib/src/realtime_channel.dart Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants