Repository navigation
Async codegen - #3951
Async codegen#3951sbiscigl wants to merge 4 commits into
Conversation
219eb33 to
07d1ce4
Compare
07d1ce4 to
166f51a
Compare
| @@ -566,25 +566,44 @@ namespace Aws | |||
| } | |||
| send(request, [self](std::shared_ptr<HttpResponse> response) | |||
| { | |||
| auto outcome = self->evaluate(response); | |||
There was a problem hiding this comment.
how come we moved evaluate off of the crt thread? my only concern is if the submit task is rejected we wont surface the correct error if there was one, whereas previously we would still evaluate the response
There was a problem hiding this comment.
moved it because evaluate is sdk work (error parsing, retry strategy, retry handlers, interceptors) and we only want i/o on the loop, but you're right about rejection, so on a rejected submit we'll just run it inline so the real outcome still comes back. fwiw main dropped the handler entirely in that case, so this ends up better than before.
| using IsEventStream = std::integral_constant<bool, IsEventStreamOperation<OperationFuncT>::value>; | ||
| const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this); | ||
| auto operationGuard = Aws::MakeShared<Aws::Utils::RAIICounter>(AwsServiceClientT::GetAllocationTag(), m_operationsProcessed, &m_shutdownSignal); | ||
| auto asyncIOOperationGuard = Aws::MakeShared<Aws::Utils::RAIICounter>(AwsServiceClientT::GetAllocationTag(), m_asyncIOOperationsProcessed, &m_shutdownSignal); |
There was a problem hiding this comment.
i think this can deadlock on shutdown. both guards live in the Bind closure, so they stay alive until the handler returns and the closure is destroyed. if the handler drops the last client ref inside its body, ShutdownSdkClient would be stuck on m_asyncIOOperationsProcessed, since it's waiting on the guard owned by the handler that's currently running. i see the test AsyncHandlerHoldingTheLastClientReferenceCompletes passes because clang happens to destroy the captures in reverse capture order but thats not enforced by the cpp standard.
my thought is we could release the guards right before calling the handler (something like a intermediary object that takes the guards and calling guards.reset() in the closure before deliver(...))? however, i will not block on this
There was a problem hiding this comment.
yep you're right, and it's worse than capture order, a handler that drops the last ref inside its body hangs every time since shutdown's async io wait has no timeout. release drops its guard before the handler runs, so we'll do the same here and reset both guards right before deliver, plus a test that drops the last ref inside the handler.
| { | ||
| while (pClient->m_asyncIOOperationsProcessed.load() != 0) | ||
| { | ||
| pClient->m_shutdownSignal.wait_for(lock, std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
another specific but problematic scenario: if the client is configured with a 1-thread PooledThreadExecutor and a handler destroys the client while another call is still in flight, shutdown would deadlocks
- call A's response completes on the only worker and runs A's handler, which drops the last client ref, so
ShutdownSdkClientruns on that worker - this loop waits on
m_asyncIOOperationsProcessed, since B's guard is still held - B's response (or its timeout) arrives on the event loop, and
CRTAttemptLoopsubmits B's evaluate to the executor (CRTHttpClient.cpp:569), so it gets queued behind the blocked worker - B's guard is only released after B's handler runs, which needs the worker, so neither side can make progress
i think the underlying issue is that shutdown blocks a worker while completions need a worker to finish. maybe once shutdown starts, completions could run inline on the event loop (my previous comment)? not sure if thats the right fix but would also be good to have a test for this with a 1-thread pool.
| return TracingUtils::MakeCallWithTiming<${operation.name}Outcome>( | ||
| #end |
There was a problem hiding this comment.
do we even need the else anymore doesnt AsyncOperationState::MakeCallWithTiming fallback to this anyway? same thing for SmithyServiceOperationsSource.vm
db4bf7b to
aba183d
Compare
Description of changes:
Async operations in the SDK hold an executor thread for the entire request. So a client can only have as many calls in flight as it has threads, and those threads spend most of their time waiting on the network. This change lets async operations give the thread back while the request is in flight.
With
AWS_CRT_HTTP_USE_ASYNC_IOon and the CRT HTTP client, every generated async operation prepares its request on the executor, hands it to the CRT event loop, and only uses a thread again once the response arrives. Callers don't change anything, and with the flag off the SDK behaves exactly as it does today.sequenceDiagram participant App participant Exec as Executor participant CRT as Event loop App->>Exec: XXXAsync(request) Exec->>CRT: prepare the request, then hand it off Note over Exec: thread is free CRT->>Exec: response Exec->>App: handler(outcome)This covers every generated client without any per-operation code. Smithy sync calls also now block on the caller like the legacy clients do, which fixes a crash in flag-on streaming calls.
The first commit has the core and codegen changes and is the one to review. The other two regenerate the clients.
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.