Sort out token refresh and saving synchronization II #102
Conversation
This behavior is now handled using NetworkingJobHandler
michpohl
changed the title
michpohl
changed the title
Now that we define the scope externally, this is needed so tests can properly control these coroutines
michpohl
force-pushed
the
michael/Sort-out-token-refresh-and-saving-synchronization-II
branch
from
855cd98
to
b252da8
Compare
| @@ -78,6 +80,7 @@ internal class LoginRepository constructor( | |||
| } | |||
|
|
|||
| suspend fun setCredentials(credentials: Credentials, refreshToken: String? = null) { | |||
| networkingJobHandler.cancelAllJobs() | |||
There was a problem hiding this comment.
I think there's a problem with this. You're going to cancel getCredentials jobs that have been added to the collection before getting here, yes. But that doesn't cover jobs that are added after this line is executed. Those will be capable of overwriting the credentials set by this method, even if the corresponding call to getCredentials happened before. You would need to run job cancellation within the mutex too I think, so that you achieve what my understanding of what you want is - have token retrieval and write run exclusively with each other. But that will give you a new problem - if there's a write request while ownership of the mutex is held by a retrieval request, it needs to bypass the wait by cancelling the existing ownership of the mutex. That may be tricky to achieve with this approach I think.
I think the easiest approach to do what you need is to rework concurrency management in the sdk to use Handler. It's a queue where you can send runnables. It's backed by a single thread, so it's thread-safe by default. You'll have two types of runnables - one for getCredentials and one for setCredentials. If you receive a request for setCredentials, you add the runnable to the end of the queue - if it's for setCredentials, you do removeCallbacks for your currently queued setCredentials Runnable if any, save the new Runnable as your current one so you can remove it later, and post it at the front of the queue so it runs before any other getCredentials requests.
Thoughts?
There was a problem hiding this comment.
Well, mainly I am trying to pragmatically present a solution for a specific issue that occurs only when using our "backdoor" setCredentials (and only when using during certain UI tests). Both my attempts solve the problem (I verified) without causing new issues elsewhere.
The other approach avoided the problems you suspect this one here has, by choosing a less "correct" approach than this here. For this edge case that is perfectly fine imho.
It's fine to disagree with my solutions though, and since you probably have had more exposure to coroutine concurrency management in the player, and appear you know how to solve this issue right, would you mind creating a proposal?
There was a problem hiding this comment.
There also is the option to pick one solution, and creating a task to improve the situation in a more general approach, which might be the most pragmatic.
This PR adds a
NetworkingJobHandlerwhich can be used to cancel token refresh calls.This is needed e.g. when calling
setCredentials, as otherwise a call togetCredentialsthat was issued before, but returns after this operation can overwrite and erase the credentials just set.This is an alternative approach to solving the same problem #101 addresses.