fix: Make HttpResponse.statusCode getter & setter thread-safe

[sichanyoo]

Issue #

awslabs/aws-sdk-swift#1324
aws-amplify/amplify-swift#3708

Addresses SIM ticket /V1219649867.

Description of changes

A customer raised a SIM ticket with the following screenshot of data race warning on IDE:

To address this data race warning, use serial queue and hidden property _statusCode to make getter & setter thread-safe. Observed that the thread sanitization warning goes away when running CRT HTTP client with this change.

Scope

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[jbelkins]

Two things:

• It is not enough to just do the writes on the serial queue. Writing on the serial queue ensures that writes will not happen at the same time, but does not guarantee that reads will not happen at the same time as writes. To fully ensure no simultaneous access, reads would have to be performed on the queue as well.
• You create the serial queue as a local variable each time that the private function safelySetStatusCode is called. That means that every write to status code will happen on its own queue, and nothing will actually be deconflicted. To ensure that all writes go onto the same queue, the queue should be a property of either the request or the client engine.

[jbelkins]

A couple questions inline. Most of PR is good - just adding await where needed

[jbelkins]

See my suggestion on a more efficient way to resume execution when status code changes to a final value

[jbelkins]

See comments. Also, remember that when a continuation is used, whether the result is success or failure, it must be resumed exactly once.

So make sure that every possible path that this request could take (success, failure, cancellation before completion, etc.) results in the continuation being called. As-is, I don't see any resumption of the continuation except on the success path.

[sichanyoo]

@jbelkins RE: making sure continuation gets resumed exactly once for all code paths:

For any situation where HTTP response is returned from the server (with any status code >= 200), it's resumed exactly once.

And for any situation that involves a non-HTTP error such as the connection being dropped due to a connection error, those errors are caught by the RetryMiddleware, which is then thrown back to the user without the code execution progressing into DeserializeMiddleware (so the continuation doesn't even get instantiated).

[sichanyoo]

From offline discussion: The PRs for this ticket (partner PR: awslabs/aws-sdk-swift#1323) will be put on-hold until deeper investigation is done & it's confirmed all continuation code paths call resume() exactly once.

Also discussed: refactoring CRT HTTP client wrapper in SDK side might be a good follow-up task to this.

[jbelkins]

This is correct as far as thread-safety. Not sure how I feel about using DispatchQueue though - CRT generally does not use it, and our projects only use it where needed to interact with Foundation APIs that are based on it.

Since this fixes the problem though, I'm approving and we can revisit it later.