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

trackInflights isn't thread-safe #2288

Open
jfrodden opened this issue Nov 10, 2022 · 1 comment
Open

trackInflights isn't thread-safe #2288

jfrodden opened this issue Nov 10, 2022 · 1 comment

Comments

@jfrodden
Copy link

jfrodden commented Nov 10, 2022

Moya -> 15.0.0

Different callbacks for the same service call don't always get called when we have the trackInflights set to true. It appears as though the @Atomic property wrapper around the MoyaProvider.internalInflightRequests property only ensures atomicity of reads/writes with respect to each read or write operation, but not across combined read/write operations, so the following code from MoyaProvider.requestNormal is not atomic across the read and write as it needs to be:

if trackInflights {
    var inflightCompletionBlocks = self.inflightRequests[endpoint] // <--- this is atomic
    inflightCompletionBlocks?.append(pluginsWithCompletion)
    self.internalInflightRequests[endpoint] = inflightCompletionBlocks // <--- and this is atomic, but not with the read

    if inflightCompletionBlocks != nil {
        return cancellableToken
    } else {
        self.internalInflightRequests[endpoint] = [pluginsWithCompletion] // <--- and this is atomic, but not with the read
    }
}

and later in the same method:

if self.trackInflights {
    self.inflightRequests[endpoint]?.forEach { $0(result) } // <--- this is atomic
    self.internalInflightRequests.removeValue(forKey: endpoint) // <--- and this is atomic, but not with the read
} else ...

I think a lock on self.inflightRequests is needed for each of the if trackInflights codepaths

@jfrodden
Copy link
Author

jfrodden commented Nov 10, 2022

it looks like PR#2242 addresses exactly this issue#1085

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

No branches or pull requests

1 participant