Refactor rest handlers into coroutines #21750
Merged
Conversation
This file contains hidden or 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
…e+waitForFuture The following handlers are changed: - RestAgencyHandler - RestAdminLogHandler - RestDocumentHandler - RestGraphHandler - RestImportHandler - RestLogHandler - RestLogInternalHandler - RestMetricsHandler - RestReplicationHandler - RestTransactionHandler - RestUsageMetricsHandler
…cute+waitForFuture
…actor-rest-handlers-into-coroutines
There was a problem hiding this comment.
Pull Request Overview
This PR refactors multiple REST handlers to eliminate the legacy waiting mechanism in favor of modern C++ coroutines. Key changes include replacing functions like execute() and waitForFuture() with executeAsync() combined with co_await/co_return patterns, consistent conversion of error‐and–success paths, and corresponding adjustments to helper and utility functions.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| arangod/RestHandler/RestReplicationHandler.{h,cpp} | Converted execute() to executeAsync() and replaced waitForFuture() with co_await/co_return. |
| arangod/RestHandler/RestMetricsHandler.{h,cpp} | Refactored the metrics handler to use asynchronous coroutine patterns. |
| arangod/RestHandler/RestLog*, RestDocumentHandler.{h,cpp} | Updated various Rest*Handlers to use async and executeAsync() with coroutines. |
| arangod/RestHandler/RestImportHandler*.{h,cpp} | Changed the synchronous execute() method to a coroutine-based executeAsync(). |
| arangod/Agency/RestAgencyHandler*.{h,cpp} | Refactored agency handler methods to use coroutines, including changes to pollIndex and error reporting. |
Comments suppressed due to low confidence (1)
arangod/RestHandler/RestLogHandler.h:39
- The new function name 'executeAsync' clearly communicates its async nature; ensure that similar naming conventions are uniformly applied across all refactored handlers.
auto executeAsync() -> futures::Future<futures::Unit> override;
|
TSan tests successful: https://app.circleci.com/pipelines/github/arangodb/arangodb/34478 |
|
ASan/LSan/UBsan tests successful: |
johann-listunov
approved these changes
May 13, 2025
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.
Scope & Purpose
As a prerequisite to rewriting the RestHandler's state machine (
runHandlerStateMachine()) into a coroutine, which in turn is a prerequisite to e.g. make AQL setup & shutdown operations asynchronous, we need to get rid of existing async/WAITING glue code, namely thewaitForFuture()methods ofRestHandler.This PR refactors most existing RestHandlers using that, so they consistently use
async<T>orfutures::Future<T>instead ofWAITINGmechanisms, starting at implementingexecuteAsync()instead ofexecute(). This gets rid of both anywaitForFuturecalls, and any need forRestStatus::WAITING, and thus mostly ofRestStatusas a whole.The only missing RestHandlers are the AQL-related ones: RestCursorHandler and its derivatives RestSimpleHandler and RestSimpleQueryHandler, and RestAqlHandler. This will be adressed together with the refactored state machine.