-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Add async interface for Graph running, make dagrs friendlier for async runtime #115
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an async interface to the Graph execution API, enabling dagrs to work within existing async runtimes without causing "nested runtime" panics. Previously, calling Graph::start() from within an async context would fail because it internally creates a new Tokio runtime.
- Introduces
async_start()method as the async-native interface for graph execution - Refactors
start()to become a thin wrapper that creates a runtime and callsasync_start() - Moves the runtime creation logic from inside the execution path to the synchronous wrapper
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .block_on(async { self.async_start().await }) | ||
| } | ||
| /// This function is used for the execution of a single dag with async. | ||
| pub async fn async_start(&mut self) -> Result<(), GraphError> { |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name async_start doesn't follow Rust async naming conventions. Standard practice in the Rust ecosystem is to avoid async_ prefixes for async methods. For example, reqwest uses send() (async) vs send() (sync isn't provided), and tokio's APIs don't use async_ prefixes.
Consider renaming this method to simply run() (making it public) since it's the async version, and the internal run() method could be renamed to something like run_internal() or execute_graph() to avoid the name conflict. This would align with the PR description's suggestion of exposing "async fn run" and is more idiomatic.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Motivation
Currently, the
Graph::startmethod in dagrs exposes only a synchronous API, which internally creates and runs a new Tokio runtime:This design works well for simple, standalone, or testing scenarios where no async runtime is already running.
Problem
However, when using dagrs in a context where a Tokio runtime already exists—such as within an async main function, an async service framework, or other async applications—calling
Graph::startwill panic at runtime with an error like:This makes dagrs incompatible with a growing number of modern Rust applications that rely on async/await and long-lived runtimes.
Why an Async Interface Is Needed
async fninterface allows dagrs to execute in any async context, avoiding nested tokio runtime panics altogether..awaitenables non-blocking scheduling, granting the broader application full control over task orchestration, cancellation, and concurrency limits.asyncfor granular, parallel, and non-blocking testing or scheduling.async fn runandfn startmirrors best practices in Rust async ecosystem (see: [reqwest], [sqlx]), enabling maximum usability for different user groups.Proposed Solution
async fn run_async(&mut self)(or simplyasync fn run) API.start()remain for sync users, but make it a thin wrapper that simply spins up a runtime and calls.run_async().await.