Cancel S3 CRT requests if should not continue #3231
Conversation
SergeyRyabinin
force-pushed
the
sr/cancelS3CrtReq
branch
from
860e582 to
f370835
Compare
SergeyRyabinin
force-pushed
the
sr/cancelS3CrtReq
branch
from
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
| @@ -13,10 +29,17 @@ static int S3CrtRequestHeadersCallback(struct aws_s3_meta_request *meta_request, | |||
| userData->response->AddHeader(StringUtils::FromByteCursor(header.name), StringUtils::FromByteCursor(header.value)); | |||
| } | |||
| 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.
SergeyRyabinin
force-pushed
the
sr/cancelS3CrtReq
branch
from
c8c42ac to
277daf4
Compare
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.