Feature/frem/check bearer token #138
Conversation
fremartini
force-pushed
the
feature/frem/checkBearerToken
branch
from
0e2f402
to
3aedb13
Compare
fremartini
force-pushed
the
feature/frem/checkBearerToken
branch
3 times, most recently
from
4bfaba3
to
4f0d98f
Compare
fremartini
force-pushed
the
feature/frem/checkBearerToken
branch
from
4f0d98f
to
e01b29d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #138 +/- ##
===========================================
- Coverage 20.00% 16.21% -3.79%
===========================================
Files 3 3
Lines 30 37 +7
===========================================
Hits 6 6
- Misses 24 31 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@fremartini (Cc @marfavi ) I'm "reopening" the discussion of this implementation as I think there are some design questions which I am unsure has been addressed or have been taken into consideration in the code review :).
Just correct me if my assumptions are wrong 😊 . Let's discuss the question in the comments and see if issues needs to be created :)
| import 'package:coffeecard/data/storage/secure_storage.dart'; | ||
| import 'package:coffeecard/service_locator.dart'; | ||
|
|
||
| class ReactivationAuthenticator extends Authenticator { |
There was a problem hiding this comment.
Open question: Have you considered the thread-safety of this class?
I know @TTA777 has introduced parallelism of REST calls which might lead to a scenario where multiple threads end in the authenticator class. This might cause a scenario where read/writes are not visible to the two threads (I think 🤔 )
There was a problem hiding this comment.
So I just tried to find out if secure_storage is intended to be thread safe, and it would appear so. According to this issue only versions before 3.3.5 should have issues, and we are on 5.0.2. With that said, there exists an open issue on secure_storage where it might be unable to read from storage, if the user restored the app from a backup. So it might be prudent to at least wrap our calls to the storage in a try catch as suggested in that issue
There was a problem hiding this comment.
My concern is both the secure storage but more the fields (retry count and denounce) since these are not visible across threads (if we have a thread issue, that's is).
There was a problem hiding this comment.
there can arise problems when the two calls on the receiptsPage both return 401 where two attempts to refresh the token are made. I have made some changes here (https://github.com/AnalogIO/coffeecard_app/tree/fix/frem/checkBearerToken) that attempt to address this issue
There was a problem hiding this comment.
I assume you are thinking of the static fields you have introduced in the ReactivationAuthenticator?
I understand why you came up with the idea but it would actually provide more unwanted side effects. First, static does not guarantee memory visibility across threads (at least in Java).
Also and maybe more importantly, the Shiftplanning API also uses an Authenticator. Introducing static fields would share the retry count and debouncer between both APIs. We don't want that!
That was a little thread safety rant, sorry 😅
I want to bring some good news though. I have tried reading up on the Dart concurrency model. Dart runs single threaded by standard in what is called an isolate (memory is isolated and not shared). My understanding is that unless you explicitly create several isolate to make multi threaded code, the code will always be single threaded.
My first thought is therefore that we don't have a thread issue here and no risk of dirty read/writes.
Do you agree on my understanding? @fremartini @TTA777
Links
- Dart Concurrency documentation
- Dart: how to manage concurrency in async function This link link goes again what I'm saying 🤔
closing #68