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

Stop spawning Tasks inside of Combine sink closure. #3700

Open
pixlwave opened this issue Jan 23, 2025 · 0 comments
Open

Stop spawning Tasks inside of Combine sink closure. #3700

pixlwave opened this issue Jan 23, 2025 · 0 comments
Labels

Comments

@pixlwave
Copy link
Member

#3699 highlighted an issue with this pattern where there's a race condition when

  1. The Task uses the value from the publisher
  2. The async method called awaits something and then uses that value.
  3. The publisher is called multiple times in close succession.

This is obvious when you think about it, but this was the first instance where we saw the race condition in action. At this time, searching for the following regex reveals 27 instances of Tasks inside of .sinks.

\.sink\s*\{[^\}]*?(?:\s*\{[^\}]*?\}[^\}]*?)*\s*Task\s*\{(?:[^{}]*|\{[^{}]*\})*\}

Note about the regex:

It needs some work, as it never finishes searching (either in Xcode or via SwiftLint). A faster one is as follows, but it doesn't find instances where there's a block before the task:

\.sink\s*\{[^\}]*?Task\s*\{(?:[^{}]*|\{[^{}]*\})*\}

Looking at the 27 instances only 3 of them (at the time of writing) really meet the requirements for being an issue:

signalCancellable = client.signals.sink { [weak self] signal in
Task {
do {
try await self?.handleSignal(signal)
} catch {
MXLog.error(error.localizedDescription)
}
}
}

signalCancellable = client.signals.sink { [weak self] signal in
Task {
do {
try await self?.handleSignal(signal)
} catch {
MXLog.error(error.localizedDescription)
}
}
}

.sink { [weak self] blockedUsers in
guard let blockedUsers else { return }
Task { await self?.updateUsers(blockedUsers) }
}

We could go about fixing this in 2 ways:

  • Re-write all of the sinks to use the for await in which would be tedious.
  • Add an asyncSink method on Publisher that wraps everything up neatly make sure we finish processing one value before the next and use this instead.

Either way we should also add a SwiftLint rule to prevent us doing this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant