pointfreeco / swift-concurrency-extras

Useful, testable Swift concurrency.
MIT License
343 stars 23 forks source link

Lock async stream inits #11

Closed stephencelis closed 1 year ago

stephencelis commented 1 year ago

Fixes #10.

@mansatgigset Can you take this branch for a spin to confirm it fixes the issue for you?

mansatgigset commented 1 year ago

I have tested the branch, and it seems to work fine. Note though, that this crash is quite random and infrequent on later OS versions, and I don't currently have access to run this on the older Intel macOS 12.x system (or even know if that crash is related to this). I think this fix is valid no matter, so please go ahead and merge and create a new release.

stephencelis commented 1 year ago

@mansatgigset Would be happy to add an availability check if we can track down the runtimes involved. Any idea which are affected, or when the fix was applied?

mansatgigset commented 1 year ago

Well, the crash we have seen on macOS 12.x (Intel), that we can't debug (due to the difficulty to debug older macOS versions), gave this short stack trace:

Exception Type:        EXC_BAD_INSTRUCTION (SIGILL)
Exception Codes:       0x0000000000000001, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Reason:    Namespace SIGNAL, Code 4 Illegal instruction: 4

Thread 4 Crashed:
0   libswiftCore.dylib              0x00007ff828033245 _assertionFailure(_:_:file:line:flags:) + 421 (AssertCommon.swift:132)
1   libswift_Concurrency.dylib      0x00007ffb34060b64 AsyncStream._Storage.next(_:) + 532 (AsyncStreamBuffer.swift:253)
2   libswift_Concurrency.dylib      0x00007ffb340735ec partial apply for closure apple/swift-async-algorithms#2 in AsyncStream._Storage.next() + 188 (<compiler-generated>:0)
3   libswift_Concurrency.dylib      0x00007ffb340742e1 thunk for @callee_guaranteed @async () -> (@out A?) + 1

Which was a result of an assertion in Apple's code base in previous releases.

See https://github.com/apple/swift/issues/69367

The other crash was actually caught when debugging on macOS using macOS 14.0, but I'm fairly sure I have seen it on iOS 16.x simulators as well.

It was the latter (and longer stack-trace) that had your type erasers in them (the one updated in this PR), hence why we suspected they might a reason behind the crash.

Mika5652 commented 3 months ago

@stephencelis Hello, is it possible to cherry-pick these changes to the version below 1.0.0.? I am working on quite a big project, where we still have TCA 0.59.0. I started using your extension eraseToStream and I am now afraid of crashes when I found this fix. I didn't notice any crashes at this time, but this can be because these changes are not released yet and this can be problematic because our app uses around 300k users. Yes, I can copy your fix into our project, but it seems better for me to not duplicate logic. Thanks for your reply!

stephencelis commented 3 months ago

@Mika5652 Sorry for the delay, we lost track of this. Just cut a 0.1.2 with this fix cherry-picked here: https://github.com/pointfreeco/swift-concurrency-extras/releases/tag/0.1.2

Mika5652 commented 3 months ago

@stephencelis It's alright. I know you have a lot of work. Thank you very much!