supabase / supabase-flutter

Flutter integration for Supabase. This package makes it simple for developers to build secure and scalable products.
https://supabase.com/
MIT License
703 stars 167 forks source link

Timeout on `refreshSession` #872

Closed ConProgramming closed 5 months ago

ConProgramming commented 5 months ago

I'm calling await Supabase.instance.client.auth.refreshSession().timeout(const Duration(seconds: 30)); when my app switches from not having connection to having connection. This is timing out if the "Access token (JWT) expiry time" has passed. (It defaults to 1 hour, but can reproduce this issue easily if I change it to 2 minutes)

ConProgramming commented 5 months ago

From debugging, looks like it gets stuck returning the existing future on the internal completer.

https://github.com/ConProgramming/supabase-flutter/blob/main/packages/gotrue/lib/src/gotrue_client.dart#L1071

dshukertjr commented 5 months ago

Yeah, I'm working on a fix around the session refresh flow, which should fix this issue too. Hopefully I can ship the fix soon.

ConProgramming commented 5 months ago

Worked on a PR. Testing it some, seems to work well https://github.com/supabase/supabase-flutter/pull/873

Obviously would have to remove logs to merge, but kept for readability. Let me know how this looks.

dshukertjr commented 5 months ago

What is the purpose of calling refresh session when coming back online in the first place? You shouldn't need to explicitly call refresh session on your own as the SDK will handle refreshing the session whenever needed.

ConProgramming commented 5 months ago

I was using it with a .timeout, then signing out if timed out. Because if I didn't then sdk calls were just hanging infinitely due to the internal refreshSession completer never completing.

dshukertjr commented 5 months ago

What call was hanging on what occasion? Rather than trying to change the refreshSessoin method, we should fix the root issue. You shouldn't have to be calling refreshSession in the first place.

ConProgramming commented 5 months ago

Mentioned at the top of this issue, calling refreshSession hangs when my app switches from not having connection to having connection if the "Access token (JWT) expiry time" has passed (It defaults to 1 hour, but can reproduce this issue easily if I change it to 2 minutes)

And sdk call starts hanging when this happens, since they try to call refreshSession internally

dshukertjr commented 5 months ago

@ConProgramming Yes, but why are you calling refreshSession in the first place?

ConProgramming commented 5 months ago

If I dont call refreshSession, all sdk calls still hang either way

They are all hanging on the same completer

Calling refreshSession manually confirms that is the error

dshukertjr commented 5 months ago

If I dont call refreshSession, all sdk calls still hang either way

Hangs when you call what? The API methods, correct? Then we should not mess with the refreshSession method, but rather work on a fix so that it doesn't hang, which I'm doing right now, so I ask you to wait patiently. The users should also never have to call refreshSession implicitly unless they have a very specific reason.

ConProgramming commented 5 months ago

The API methods are hanging because refreshSession is hanging, yes? The problem is that the completer never completes

dshukertjr commented 5 months ago

refreshSession isn't just hanging for no reason. It's waiting for the retries to be completed.

The problem is that the completer never completes

If you are referring to when the device is offline, then that is the expected behavior as the retries should be in progress, although after a few minutes or so it should fail.

I have a rework in progress that will make the API methods fail right away if there is no network connection while retries will be in progress in the background. Again, I ask that you wait patiently. No need to reply to this message.