change APIProvider to be an actor instead of a class #242
Conversation
nickasd
force-pushed
the
apiprovider-actor
branch
from
e8d1b48 to
f171bc2
Compare
|
I just followed your instructions on how to run the tests and noticed that I still had to change the tests. I don't know if I'm doing something wrong, but running But running Then I was able to run the tests. But they fail in the Terminal: In fact, when opening I had to change the target to |
Code Coverage Report
SwiftLint found issues
Generated by 🚫 Danger Swift against f171bc2 |
There was a problem hiding this comment.
Thanks a lot for your contribution! Unfortunately, I don't think this will solve the issue. It will make it impossible to run requests in parallel, which is a decrease in performance.
Instead, we should fix this inside JWTRequestsAuthenticator. In hindsight, without the use of an actor since we're still supporting non-async calls.
To do this, we can add a lock queue around all logic inside createToken(). A serial DispatchQueue with a custom label will do for now since it's unlikely to result in performance issues.
Let me know if you're able to pick this up!
Thanks for checking. Are you sure that requests cannot run in parallel? I just tried changing with this When running my app, which loads different resources at the same time, I get this output: Isn't this a sign that requests run in parallel? |
Hi @nickasd 👋🏼 I'll chime in here. If ApiProvider is an actor and you make all the requests from a single ApiProvider instance I will expect them to be executed sequentially. To follow @AvdLee's suggestion I think we can fix the underlying issue in private let dispatchQueue = DispatchQueue(label: "JWTRequestsAuthenticator", attributes: .concurrent)
private func createToken() throws -> JWT.Token {
try dispatchQueue.sync(flags: .barrier) {
if let cachedToken = cachedToken, !cachedToken.isExpired {
return cachedToken
}
let token = try jwtCreator.signedToken(using: apiConfiguration.privateKey)
cachedToken = token
return token
}
}I can make a PR to make this easier to read. Let me know what you think 🙏🏼 |
|
Hey @mathiasemil, that should work as well. But why use a concurrent queue if you only submit barriers to it, and not use a standard serial queue? I was just thinking, since what we want to protect here is concurrent read and write to the |
I guess in this case there is no reason to not use a serial queue instead. So that sounds like a better approach 👍🏼 |
|
I just realized now that it also checks for I'll update the PR. |
|
Continued in #246. |
Solves #238.