Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Refactor RestHandler state machine into a coroutine#21787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
goedderz wants to merge77 commits intodevel
base:devel
Choose a base branch
Loading
fromfeature/refactor-resthandler-state-machine-into-coro

Conversation

goedderz
Copy link
Member

@goedderzgoedderz commentedMay 27, 2025•
edited
Loading

Scope & Purpose

Refactor the RestHandler's state machine into a coroutine. This is foundational work to allow rewriting more code into (asynchronous) coroutines.

  • 🔨 Refactoring/simplification

Checklist

  • Covered by existing tests

goedderzand others added30 commitsFebruary 13, 2025 14:09
- Temporarily disabled LogContext (ScopedValue in rest handler state  machine)- Made more stuff async- Fixed an issue by moving a coro from Future to async, because futures  don't pass ExecContext as coros (just as an awaitable)- Fixed an issue by moving WAITING<->coro adapter further up the stack
…e+waitForFutureThe following handlers are changed:- RestAgencyHandler- RestAdminLogHandler- RestDocumentHandler- RestGraphHandler- RestImportHandler- RestLogHandler- RestLogInternalHandler- RestMetricsHandler- RestReplicationHandler- RestTransactionHandler- RestUsageMetricsHandler
…ub.com:arangodb/arangodb into feature/refactor-resthandler-state-machine-into-coro
@goedderzgoedderz marked this pull request as ready for reviewJune 27, 2025 11:27
@goedderzgoedderz requested a review froma teamJune 27, 2025 17:57
Copy link
Member

@neunhoefneunhoef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This looks overall very good and is going exactly in the right direction. I have added some minor comments, some of which need addressing. And a CHANGELOG entry is missing. In particular I checked that the log context is now in the async context and will thus be moved from thread local variable to thread local variable if a coroutine is suspended on one thread and resumed on another. Therefore it is now allowed to use ScopedValue and log contexts in coroutines (with its lifetime bound to the coroutine context object).

add_library(arango_async INTERFACE)
target_include_directories(arango_async INTERFACE
add_library(arango_async STATIC
include/Async/async.h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why are these header files listed here? Usually, we seem to only add the .cpp files.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

They are dependencies, the library needs to be rebuilt if the headers change. That being said, I think the build system does detect them as such anyway, so it shouldn't be necessary for that. Where it does make a difference is for IDEs like Visual Studio, when the CMake files are used as a basis for a project. In particular if the header files don't have a corresponding source file of the same name, in which case they should at least be detected by heuristics.

All in all, while I don't think it's hugely important, I do think it's generally preferable to add header files as well.

Copy link
Contributor

@jvolmerjvolmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This makes the code much better readable, really cool! I added some questions and minor comments.

goedderzand others added17 commitsJuly 13, 2025 12:32
Co-authored-by: Julia Volmer <julia.volmer@arangodb.com>
Co-authored-by: Julia Volmer <julia.volmer@arangodb.com>
@goedderzgoedderz requested a review fromneunhoefJuly 17, 2025 15:16
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jvolmerjvolmerjvolmer approved these changes

@neunhoefneunhoefAwaiting requested review from neunhoef

Requested changes must be addressed to merge this pull request.

Assignees

@goedderzgoedderz

Labels
Projects
None yet
Milestone
devel
Development

Successfully merging this pull request may close these issues.

3 participants
@goedderz@neunhoef@jvolmer
[8]ページ先頭
©2009-2025 Movatter.jp