Cancel S3 CRT requests if should not continue - #3231
Conversation
860e582 to
f370835
Compare
f370835 to
c8c42ac
Compare
| putObjectRequest2.SetBucket(m_bucketName); | ||
| putObjectRequest2.SetKey(TEST_KEY); | ||
| std::atomic<bool> shouldContinueAtomic{true}; | ||
| putObjectRequest2.SetContinueRequestHandler([&shouldContinueAtomic](const HttpRequest*) { return shouldContinueAtomic.load(); }); |
There was a problem hiding this comment.
I understand that the atomic flag once set false will prompt the following code in the callback
userData->s3CrtClient->CancelCrtRequestAsync(meta_request);
If the request is called twice, is there safeguard to protect duplicate cancellation of same request in crt?
There was a problem hiding this comment.
According to the CRT developers, CRT is safe on calling the cancellation method on the same task handle twice, or even on an invalid task handle pointer.
There was a problem hiding this comment.
crt is fine with stuff being called twice as long as the handle is still valid and meta req has not been released
| { | ||
| wrappedData->fn(wrappedData->data); | ||
| } | ||
| wrappedData->clientShutdownSem->Release(); |
There was a problem hiding this comment.
added, thank you
| } | ||
| userData->response->SetResponseCode(static_cast<HttpResponseCode>(response_status)); | ||
|
|
||
| auto& shouldContinueFn = userData->originalRequest->GetContinueRequestHandler(); |
There was a problem hiding this comment.
though this is not added in this PR, I think we should add ptr null checks
There was a problem hiding this comment.
added, thank you!
| { | ||
| wrappedData->fn(wrappedData->data); | ||
| } | ||
| wrappedData->clientShutdownSem->Release(); |
There was a problem hiding this comment.
I moved this method from .h to .cpp, but it is added now, thank you.
sbera87
left a comment
sbera87
left a comment
There was a problem hiding this comment.
Looks good overall. Just need some ptr null checks . Open question about effect of duplicate request cancellation
c8c42ac to
277daf4
Compare
DmitriyMusatkin
left a comment
DmitriyMusatkin
left a comment
There was a problem hiding this comment.
havent reviewed in depth and will leave cpp/style comments for someone more familiar with cpp, but from crt perspective this looks fine
4 participants
Issue #, if available:
awslabs/aws-c-s3#467
S3 CRT integration does not map ShouldContinue callback call
Description of changes:
Add calling the ShouldContinue callback
To be honest, that is not the best solution possible to add the functionality of cancel.
We are going to introduce a better API with the refactoring of the transfer manager to officially support S3 CRT client.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.