Fix race condition with aws_s3_endpoint acquire/release #209
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
source/s3_client.c
Outdated
|
|
||
| /* not calling aws_s3_endpoint_acquire() because that would grab the client lock */ | ||
| AWS_ASSERT(endpoint->client_synced_data.ref_count > 0); | ||
| ++endpoint->client_synced_data.ref_count; |
There was a problem hiding this comment.
this seems to contradict pr description that we should always hold lock on refcount changes
There was a problem hiding this comment.
I'll change the comment to be more clear
we already HAVE the lock here in this code, is why we're not calling that other function
There was a problem hiding this comment.
actually, I did something else
took away comment and made the code self-documenting
we DO call the function now, but pass in an arg to say that we're holding the lock
DmitriyMusatkin
approved these changes
Sep 14, 2022
TingDaoK
approved these changes
Sep 14, 2022
| struct aws_ref_count ref_count; | ||
| struct { | ||
| /* This is NOT an atomic ref-count. | ||
| * The endpoint lives in hashtable: `aws_s3_client.threaded_data.endpoints` |
There was a problem hiding this comment.
trivial: aws_s3_client.threaded_data.endpoints is it out of date?
graebm
added a commit
to awslabs/aws-crt-java
that referenced
this pull request
Sep 14, 2022
see: awslabs/aws-c-s3#209 Update submodules to latest: aws-c-io: v0.13.2 -> v0.13.4 aws-c-s3: v0.1.46 -> v0.1.48 aws-lc: v1.1.0 -> v1.2.0 s2n: v1.3.20 -> v1.3.21
graebm
added a commit
to awslabs/aws-crt-java
that referenced
this pull request
Sep 14, 2022
see: awslabs/aws-c-s3#209 Update submodules to latest: aws-c-io: v0.13.2 -> v0.13.4 aws-c-s3: v0.1.46 -> v0.1.48 aws-lc: v1.1.0 -> v1.2.0 s2n: v1.3.20 -> v1.3.21
graebm
added a commit
to awslabs/aws-crt-cpp
that referenced
this pull request
Sep 15, 2022
*Issue #:* awslabs/aws-c-s3#202 *Description of changes:* update aws-c-s3 submodule, containing fix from PR awslabs/aws-c-s3#209
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue:
#202
There are race conditions around the removal of endpoints from the client's hashtable
Description of changes:
aws_s3_endpointRambling:
This isn't our first time trying to fix these race conditions. See:
We also considered moving the endpoint hashtable out of
synced_dataand ontothread_data, and changingaws_s3_acquire()/release()calls so they become scheduled tasks under the hood. But this would have been a significant refactor. Once I'd spent 2 days staring at this code I understood it well enough to (hopefully) fix it with a small change.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.