Skip to content

Commit 2e68567

Browse files
Cameron Evansmeta-codesync[bot]
authored andcommitted
Refactor underlying implementation of PythonAsyncProcessor::dispatchRequest* to coroutines.
Summary: As titled. The motivation for this change is to make the follow-up diff easier to write. Specifically, the serializedRequest parameter would need to be used in multiple lambdas with the current SemiFuture implementation, and guaranteeing the lifetime across those lambdas turned out to be tricky (I got it wrong multiple times). Instead of doing that the hard way, the whole implementation of the function can be converted to a coroutine inside a single lambda. With that, lifetime management of `serializedRequest` becomes trivial. This also made it easy to collapse the duplicate calls to `handlePythonServerCallback*` into a single call. ref: https://www.internalfb.com/wiki/Scuba_Internal_Team/Performance_and_Optimization/Future_to_Coroutine_Recipe_Book/ Reviewed By: ahilger Differential Revision: D86481020 fbshipit-source-id: 32e96951ec87df2f9a4729c3cb45806dfc550653
1 parent 3e8f141 commit 2e68567

1 file changed

Lines changed: 99 additions & 76 deletions

File tree

third-party/thrift/src/thrift/lib/python/server/PythonAsyncProcessor.cpp

Lines changed: 99 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -350,23 +350,28 @@ folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestOneway(
350350
apache::thrift::SerializedRequest serializedRequest,
351351
apache::thrift::RpcKind kind,
352352
apache::thrift::HandlerCallbackBase::Ptr callback) {
353-
if (!shouldProcessServiceInterceptorsOnRequest(*callback)) {
354-
return handlePythonServerCallbackOneway(
355-
protocol, ctx, std::move(serializedRequest), kind, std::move(callback));
356-
}
357-
return processServiceInterceptorsOnRequest(
358-
*callback, emptyInterceptorsArguments())
359-
.semi()
360-
// see discussion below about why we don't use `defer`
361-
.deferValue([this,
353+
return folly::coro::co_invoke(
354+
[this,
355+
protocol,
356+
ctx,
357+
serializedRequest = std::move(serializedRequest),
358+
kind,
359+
callback = std::move(
360+
callback)]() mutable -> folly::coro::Task<folly::Unit> {
361+
if (shouldProcessServiceInterceptorsOnRequest(*callback)) {
362+
// see discussion below about why we don't handle exception
363+
// here.
364+
co_await processServiceInterceptorsOnRequest(
365+
*callback, emptyInterceptorsArguments());
366+
}
367+
co_return co_await handlePythonServerCallbackOneway(
362368
protocol,
363369
ctx,
364-
request = std::move(serializedRequest),
370+
std::move(serializedRequest),
365371
kind,
366-
callback](auto&&) mutable {
367-
return handlePythonServerCallbackOneway(
368-
protocol, ctx, std::move(request), kind, std::move(callback));
369-
});
372+
std::move(callback));
373+
})
374+
.semi();
370375
}
371376

372377
folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestStreaming(
@@ -377,23 +382,28 @@ folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestStreaming(
377382
::apache::thrift::HandlerCallback<::apache::thrift::ResponseAndServerStream<
378383
std::unique_ptr<::folly::IOBuf>,
379384
std::unique_ptr<::folly::IOBuf>>>::Ptr callback) {
380-
if (!shouldProcessServiceInterceptorsOnRequest(*callback)) {
381-
return handlePythonServerCallbackStreaming(
382-
protocol, ctx, std::move(serializedRequest), kind, std::move(callback));
383-
}
384-
return processServiceInterceptorsOnRequest(
385-
*callback, emptyInterceptorsArguments())
386-
.semi()
387-
// see discussion below about why we don't use `defer`
388-
.deferValue([this,
385+
return folly::coro::co_invoke(
386+
[this,
387+
protocol,
388+
ctx,
389+
serializedRequest = std::move(serializedRequest),
390+
kind,
391+
callback = std::move(
392+
callback)]() mutable -> folly::coro::Task<folly::Unit> {
393+
if (shouldProcessServiceInterceptorsOnRequest(*callback)) {
394+
// see discussion below about why we don't handle exception
395+
// here.
396+
co_await processServiceInterceptorsOnRequest(
397+
*callback, emptyInterceptorsArguments());
398+
}
399+
co_return co_await handlePythonServerCallbackStreaming(
389400
protocol,
390401
ctx,
391-
request = std::move(serializedRequest),
402+
std::move(serializedRequest),
392403
kind,
393-
callback](auto&&) mutable {
394-
return handlePythonServerCallbackStreaming(
395-
protocol, ctx, std::move(request), kind, std::move(callback));
396-
});
404+
std::move(callback));
405+
})
406+
.semi();
397407
}
398408

399409
folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestSink(
@@ -405,23 +415,28 @@ folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestSink(
405415
std::unique_ptr<::folly::IOBuf>,
406416
std::unique_ptr<::folly::IOBuf>,
407417
std::unique_ptr<::folly::IOBuf>>>::Ptr callback) {
408-
if (!shouldProcessServiceInterceptorsOnRequest(*callback)) {
409-
return handlePythonServerCallbackSink(
410-
protocol, ctx, std::move(serializedRequest), kind, std::move(callback));
411-
}
412-
return processServiceInterceptorsOnRequest(
413-
*callback, emptyInterceptorsArguments())
414-
.semi()
415-
// see discussion below about why we don't use `defer`
416-
.deferValue([this,
418+
return folly::coro::co_invoke(
419+
[this,
420+
protocol,
421+
ctx,
422+
serializedRequest = std::move(serializedRequest),
423+
kind,
424+
callback = std::move(
425+
callback)]() mutable -> folly::coro::Task<folly::Unit> {
426+
if (shouldProcessServiceInterceptorsOnRequest(*callback)) {
427+
// see discussion below about why we don't handle exception
428+
// here.
429+
co_await processServiceInterceptorsOnRequest(
430+
*callback, emptyInterceptorsArguments());
431+
}
432+
co_return co_await handlePythonServerCallbackSink(
417433
protocol,
418434
ctx,
419-
request = std::move(serializedRequest),
435+
std::move(serializedRequest),
420436
kind,
421-
callback](auto&&) mutable {
422-
return handlePythonServerCallbackSink(
423-
protocol, ctx, std::move(request), kind, std::move(callback));
424-
});
437+
std::move(callback));
438+
})
439+
.semi();
425440
}
426441

427442
folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestBidi(
@@ -434,23 +449,28 @@ folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestBidi(
434449
std::unique_ptr<::folly::IOBuf>,
435450
std::unique_ptr<::folly::IOBuf>,
436451
std::unique_ptr<::folly::IOBuf>>>::Ptr callback) {
437-
if (!shouldProcessServiceInterceptorsOnRequest(*callback)) {
438-
return handlePythonServerCallbackBidi(
439-
protocol, ctx, std::move(serializedRequest), kind, std::move(callback));
440-
}
441-
return processServiceInterceptorsOnRequest(
442-
*callback, emptyInterceptorsArguments())
443-
.semi()
444-
// see discussion below about why we don't use `defer`
445-
.deferValue([this,
452+
return folly::coro::co_invoke(
453+
[this,
454+
protocol,
455+
ctx,
456+
serializedRequest = std::move(serializedRequest),
457+
kind,
458+
callback = std::move(
459+
callback)]() mutable -> folly::coro::Task<folly::Unit> {
460+
if (shouldProcessServiceInterceptorsOnRequest(*callback)) {
461+
// see discussion below about why we don't handle exception
462+
// here.
463+
co_await processServiceInterceptorsOnRequest(
464+
*callback, emptyInterceptorsArguments());
465+
}
466+
co_return co_await handlePythonServerCallbackBidi(
446467
protocol,
447468
ctx,
448-
request = std::move(serializedRequest),
469+
std::move(serializedRequest),
449470
kind,
450-
callback](auto&&) mutable {
451-
return handlePythonServerCallbackBidi(
452-
protocol, ctx, std::move(request), kind, std::move(callback));
453-
});
471+
std::move(callback));
472+
})
473+
.semi();
454474
}
455475

456476
folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestResponse(
@@ -459,29 +479,32 @@ folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequestResponse(
459479
apache::thrift::SerializedRequest serializedRequest,
460480
apache::thrift::RpcKind kind,
461481
HandlerCallback<std::unique_ptr<folly::IOBuf>>::Ptr callback) {
462-
if (!shouldProcessServiceInterceptorsOnRequest(*callback)) {
463-
return handlePythonServerCallback(
464-
protocol, ctx, std::move(serializedRequest), kind, std::move(callback));
465-
}
466-
467-
return processServiceInterceptorsOnRequest(
468-
*callback, emptyInterceptorsArguments())
469-
.semi()
470-
// It may appear that we're discarding exception result of onRequest
471-
// interceptor, but it's actually caught via throw_wrapped, which
472-
// invokes sendException to report the callback completed with exception,
473-
// thereby invoking the onResponse interceptor.
474-
// Explicitly handling it here via `defer` + `callback->exception(...)`
475-
// results in double invocation.
476-
.deferValue([this,
482+
return folly::coro::co_invoke(
483+
[this,
484+
protocol,
485+
ctx,
486+
serializedRequest = std::move(serializedRequest),
487+
kind,
488+
callback = std::move(
489+
callback)]() mutable -> folly::coro::Task<folly::Unit> {
490+
if (shouldProcessServiceInterceptorsOnRequest(*callback)) {
491+
// It may appear that we're discarding exception result of
492+
// onRequest interceptor, but it's actually caught via
493+
// throw_wrapped, which invokes sendException to report the
494+
// callback completed with exception, thereby invoking the
495+
// onResponse interceptor. Explicitly handling it here results
496+
// in double invocation.
497+
co_await processServiceInterceptorsOnRequest(
498+
*callback, emptyInterceptorsArguments());
499+
}
500+
co_return co_await handlePythonServerCallback(
477501
protocol,
478502
ctx,
479-
request = std::move(serializedRequest),
503+
std::move(serializedRequest),
480504
kind,
481-
callback](auto&&) mutable {
482-
return handlePythonServerCallback(
483-
protocol, ctx, std::move(request), kind, callback);
484-
});
505+
std::move(callback));
506+
})
507+
.semi();
485508
}
486509

487510
folly::SemiFuture<folly::Unit> PythonAsyncProcessor::dispatchRequest(

0 commit comments

Comments
 (0)