Potential Crash: HTTPBody.collect(upTo:) is not thread safe

[LarsPetersHH]

Description

The shared resource (locked_iteratorCreated) is protected by a lock in line 386 (try checkIfCanCreateIterator()) but not protected after that – until it is protect again in line 395 (via makeAsyncIterator()).
This can lead to incosistent state where the flow continues past 386 (locked_iteratorCreated == false) and the intentional crash in makeAsyncIterator() is triggered when another thread sets locked_iteratorCreated to true in the meantime.
With enough threads calling e.g. HTTPBody.ByteChunk(collecting:, upTo:) on the same HTTPBody object, it is pretty easy to produce a crash.

In collect(upTo:) locked_iteratorCreated needs to be protected until after the async sequence was created to ensure consistency (avoiding deadlocks, of course).

My I also suggest instead of the intentional crash in HTTPBody.makeAsyncIterator() (line 346) to return an async throwing sequence which (re-)throws the error thrown from tryToMarkIteratorCreated()?
I think that would honor both the intention that a HTTPBody with IterationBehavior.single may be iterated only once and at the same time the fact that HTTPBody conforms to AsyncSequence.
The same applies to MultipartBody.makeAsyncIterator()

Reproduction

My test case in Test_HTTPBody.swift crashes reliably on my M1 Pro:

    func testThreadSafety() async throws {
        for testIteration in 0..<100 {
            print("Starting test iteration \(testIteration)")
            
            let sut = HTTPBody([HTTPBody.ByteChunk("")], length: .unknown, iterationBehavior: .single)
            let collectSuccess = expectation(description: "collectSuccess")
            let collectFailure = expectation(description: "collectFailure")
            let taskCount = 100
            
            collectFailure.expectedFulfillmentCount = taskCount - 1
            
            for _ in 0..<taskCount {
                Task(priority: .high) {
                    try? await Task.sleep(nanoseconds: 10_000_000) // makes collision a bit more likely from experience
                    
                    do {
                        _ = try await HTTPBody.ByteChunk(collecting: sut, upTo: 99)

                        collectSuccess.fulfill()
                    } catch {
                        collectFailure.fulfill()
                    }
                }
            }
            
            await fulfillment(of: [collectSuccess, collectFailure], timeout: 10, enforceOrder: false)
        }
    }

Package version(s)

main branch of swift-openapi-runtime:

.
├── swift-http-types<https://github.com/apple/[email protected]>
└── swift-docc-plugin<https://github.com/apple/[email protected]>
    └── swift-docc-symbolkit<https://github.com/apple/[email protected]>

Expected behavior

No crash.

Environment

swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx14.0

Additional information

I could provide a pull request with a fix (have to find a bit of time for that).

[czechboy0]

Hi @LarsPetersHH,

Expected behavior

No crash.

This part is unclear to me - if you create an HTTPBody with single iteration behavior, it's expected that it crashes if you try to iterate it more than once.

You're right that we're not holding the lock between the proactive check in checkIfCanCreateIterator and actually creating the async iterator, but that shouldn't matter. The proactive check is just to throw a more user-friendly error, but if you iterate the HTTPBody a second time (and the iteration behavior is single), you'll always crash. So the worst thing that could happen here is that you get a crash at the site of the first, not second iteration. Do you consider that the issue here?

[LarsPetersHH]

Hi @czechboy0

I guess there are really two parts to this report for me:

a) Intentional Crash

it's expected that it crashes if you try to iterate it more than once.

Why crash? Why not throw an error? What is the thing that is worse than a crash that the crash protects the user from in this case?

The code comment says // The crash on error is intentional here. but does not state why.

b) Thread safety
The way I read collect(upTo:), it is supposed to throw an error at iteration count >1. And that's indeed what it does when I run the same test on a single thread.

Same test, but on a single thread passes:

    func testMultipleIterations() async throws {
        for testIteration in 0..<100 {
            print("Starting test iteration \(testIteration)")
            
            let sut = HTTPBody([HTTPBody.ByteChunk("")], length: .unknown, iterationBehavior: .single)
            let collectSuccess = expectation(description: "collectSuccess")
            let collectFailure = expectation(description: "collectFailure")
            let taskCount = 100
            
            collectFailure.expectedFulfillmentCount = taskCount - 1
            
            for _ in 0..<taskCount {
                do {
                    _ = try await HTTPBody.ByteChunk(collecting: sut, upTo: 99)

                    collectSuccess.fulfill()
                } catch {
                    collectFailure.fulfill()
                }
            }
            
            await fulfillment(of: [collectSuccess, collectFailure], timeout: 10, enforceOrder: false)
        }
    }

So when the past passes on a single thread but doesn't in a concurrent situation, doesn't that mean that there is a thread safety issue?

[czechboy0]

You're right - this needs a fix to avoid crashes when only an error should be thrown.

Would you be interested in opening a PR for it, @LarsPetersHH?

[LarsPetersHH]

@czechboy0 Sure, I'll be happy to do that. Give me a few days to find the time. Maybe over the weekend.

[czechboy0]

No rush, thanks for reporting, @LarsPetersHH! 🙏

[LarsPetersHH]

@czechboy0 Here you go: apple/swift-openapi-runtime#95