feat(asyncification) #2035

aaronArinder · 2024-08-07T20:00:26Z

Resolved Conflicts: crates/rover-client/Cargo.toml crates/rover-client/src/operations/subgraph/fetch_all/runner.rs src/command/dev/do_dev.rs src/command/dev/introspect.rs src/command/dev/watcher.rs src/command/graph/introspect.rs src/command/graph/mod.rs src/command/install/mod.rs src/command/install/plugin.rs src/command/subgraph/introspect.rs src/command/subgraph/mod.rs src/command/supergraph/compose/do_compose.rs src/utils/supergraph_config.rs

jonathanrainer

Couple of things and a few questions but overall it looks great!

Cargo.toml

jonathanrainer · 2024-08-08T10:59:00Z

src/bin/rover.rs

@@ -1,5 +1,6 @@
 use robot_panic::setup_panic;
 use rover::cli::Rover;
+use tokio::runtime::Runtime;

 #[calm_io::pipefail]
 fn main() -> Result<_, std::io::Error> {


I'm going to assume there's a really good reason for this because I assume it was part of Geoffroy's PR originally, but is there a reason we create our own runtime here rather than using [tokio::main]?

😬 I'm not sure, this was part of the OG pr; I can see if it's possible, though (I have a very vague memory of trying to do this before and running into some kind of trouble, but I forget exactly what went down or even if it did go down)

ahhh, right; so, calm_io isn't async and that throws an error with tokio::main when tokio::main comes under calm_io::pipefail and I'm assuming @Geal saw that and went with the individual runtime (that's what I did, at least, until swapping the macros)

jonathanrainer · 2024-08-08T11:02:50Z

src/cli.rs

-                command.run(self.get_install_override_path()?, self.get_client_config()?)
+                command
+                    .run(self.get_install_override_path()?, self.get_client_config()?)
+                    .await
            }
            Command::Fed2(command) => command.run(self.get_client_config()?),


Is there a reason we don't await on this particular command? I can't imagine it doesn't call out to anything

Same on the other ones as well actually, not that I think we've missed something I just wanted to check!

mmm, this could be a bad merge resolution; I'll take a look at this file

yep, initial suspicion was correct: these don't call out and aren't async, so no need (or ability) to await

jonathanrainer · 2024-08-08T11:04:11Z

src/command/config/mod.rs

            Command::Auth(command) => command.run(client_config.config),
            Command::List(command) => command.run(client_config.config),
            Command::Delete(command) => command.run(client_config.config),
            Command::Clear(command) => command.run(client_config.config),


Similar question here, I know some of these reach out to studio so I'd expect they'd need to be awaited too?

same story as above; no futures here, all synchronous

jonathanrainer · 2024-08-08T11:06:10Z

src/command/dev/introspect.rs

-        .exec(&self.client, true, self.retry_period)
+        // WARNING: this changed on `main` to `true`, which I kept as `false` (async branch), but it needs to
+        // be validated
+        .exec(&self.client, false, self.retry_period)


I'm pretty sure this came from the bug we fixed for Monday.com so it would need to be changed back as that is now the correct behaviour

jonathanrainer · 2024-08-08T11:12:07Z

src/command/dev/introspect.rs

-        .exec(&self.client, true, self.retry_period)
+        // WARNING: this changed on `main` to `true`, which I kept as `false` (async branch), but it needs to
+        // be validated
+        .exec(&self.client, false, self.retry_period)


Same story here as above

rgr rgr; changed

jonathanrainer · 2024-08-08T11:51:18Z

src/command/dev/protocol/leader.rs

+        // WARNING: geal's branch had this as an awaited future for shutdown(), but we can't get
+        // that without marrking it as async, which isn't allowed, Drop can only be sync; not sure
+        // how to DRY this up


I think in this instance we can leave it as is, if we're going to drop LeaderFollower that'll probably become more of a non-issue as we move forward

sg! removing warning

Co-authored-by: Jonathan Rainer <Jonathan.rainer@apollographql.com>

dylan-apollo · 2024-08-09T16:06:02Z

crates/rover-client/src/blocking/client.rs

@@ -37,7 +36,7 @@ impl GraphQLClient {
    ///
    /// Takes one argument, `variables`. Returns an optional response.
    /// Automatically retries requests.
-    pub fn post<Q>(
+    pub async fn post<Q>(


Is the blocking module containing async functions part of the "this is WIP" disclaimer in the README?

yep! we're going to have more clean up after the next rc, which will probably make it into the actual next release

dylan-apollo · 2024-08-09T16:17:43Z

Cargo.toml

@@ -195,6 +195,8 @@ tracing = { workspace = true }
 which = { workspace = true }
 uuid = { workspace = true }
 url = { workspace = true, features = ["serde"] }
+tokio = { workspace = true, features = ["rt", "rt-multi-thread", "macros"] }


Can't highlight the line, but, seems like maybe we can get rid of all the reqwest "blocking" features now?

good call out! removing

dylan-apollo · 2024-08-09T16:23:32Z

src/command/dev/do_dev.rs

@@ -146,7 +145,7 @@ impl Dev {

            // start the interprocess socket health check in the background
            let health_messenger = follower_messenger.clone();
-            tp.spawn(move || {
+            tokio::task::spawn_blocking(move || {
                let _ = health_messenger.health_check().map_err(|_| {
                    eprintln!("{}shutting down...", Emoji::Stop);
                    std::process::exit(1);


Exiting handles still-running tasks differently than still-running threads. I don't remember the details but we'll want to test (if we haven't already) that expected things actually shut down. Notably, that the router subprocess doesn't keep on running.

I think we're good!

aaronarinder@Aarons-MBP  ~/apollo/rover   asyncification  cargo rover dev --graph-ref betelgeuse- countries@main --profile staging-test Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.27s Running `target/debug/rover dev --graph-ref 'betelgeuse-countries@main' --profile staging-test` ⌛ retrieving subgraphs remotely from betelgeuse-countries@main ✅ supergraph config loaded successfully ⚠️ Do not run this command in production! ⚠️ It is intended for local development. 🛫 starting a session with the 'countries' subgraph 🛫 starting a session with the 'name' subgraph 🛫 starting a session with the 'private-countries' subgraph 🎶 composing supergraph with Federation v2.8.2 WARN: telemetry.instrumentation.spans.mode is currently set to 'deprecated', either explicitly or via defaulting. Set telemetry.instrumentation.spans.mode explicitly in your router.yaml to 'spec_compliant ' for log and span attributes that follow OpenTelemetry semantic conventions. This option will be defa ulted to 'spec_compliant' in a future release and eventually removed altogether 🚀 your supergraph is running! head to http://localhost:4000 to query your supergraph ✅ successfully composed after adding the 'private-countries' subgraph ^C ✋ shutting down the `rover dev` session and all attached processes... ✘ aaronarinder@Aarons-MBP  ~/apollo/rover   asyncification  ps aux | grep "router" aaronarinder 58299 0.0 0.0 0 0 s006 ?+ 1:39PM 0:00.00 grep --color=auto --exc lude-dir=.bzr --exclude-dir=CVS --exclude-dir=.git --exclude-dir=.hg --exclude-dir=.svn --exclude-dir= .idea --exclude-dir=.tox router

aaronArinder · 2024-08-09T17:49:00Z

NOTE: not resolving the conflicts with main until we sort out the emoji-killification stuff (@jonathanrainer, @dylan-apollo)

jonathanrainer

LGTM

## Description We missed the removal of the blocking client in `xtask` during the [asyncification](#2035). This PR fixes that. ``` Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 12s Running `target/debug/xtask prep` ... thread 'main' panicked at /Users/dan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.1/src/runtime/blocking/shutdown.rs:51:21: Cannot drop a runtime in a context where blocking is not allowed. This happens when a runtime is dropped from within an asynchronous context. note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` ## Testing `cargo xtask prep` does not panic.

@aaronArinder

> Important: 1 potentially breaking changes below, indicated by **❗ BREAKING ❗** ## ❗ BREAKING ❗ - **The --client-timeout flag now represents the period over which we allow retries - @aaronArinder PR #2019** The documentation for this flag indicated that this was the period over which Rover would retry a command if there were retryable HTTP errors. However this was not the case due to complexities in how the client was instantiated. This has now been corrected, so the documented behaviour matches the actual behaviour. ## 🚀 Features - **Make `rover` operate asynchronously - @aaronArinder @Geal PR #2035** Removes the use of the `reqwest` blocking client allowing `rover` to operate using an asynchronous `tokio` runtime. This will bring performance improvements, particularly where working with large sets of subgraphs. - **Add `--graph-ref` to `supergraph compose` - @jonathanrainer PR #2001** Adds the same capabilities to `supergraph compose` as were added to `rover dev` in 0.25.0. You can now specify an existing Studio graphref and the command will run composition over the subgraphs specified in the graphref, as well as any overrides specified in a given supergraph config. - **Add new `rover cloud` command - @loshz PR #2008** Adds a new command to allow you to push or pull the Router config to a Cloud Router that is running in Studio - **Add new `rover cloud config validate` subcommand - @loshz PR #2055** Adds a new command enabling you to validate the Router config for a Cloud Router ## 🐛 Fixes - **Don't run IsFederatedGraph before running SubgraphFetchQuery - @glasser PR #2004** Previously we were checking IsFederatedGraph before running SubgraphFetch, but the same check is actually performed in SubgraphFetch anyway so the first call to IsFederatedSubgraph is unnecessary. - **Allow `--graph-ref` to support contract variants - @jonathanrainer PR #2036** There was a bug where using the graphref of a contract variant would cause an error about non-federated graphs. This has been resolved and now contract variant graphrefs can also be used. - **Remove last reference to blocking `reqwest` client - @loshz PR #2050** One reference to the blocking `reqwest` client had been leftover from the move to `async` operation in #2035, this was removed. - **Ensure NPM installer on Windows works correctly - @jonathanrainer PR #2059** The NPM installer on Windows had been broken because it was attempt to rename a binary from `rover` to its correct name, rather than from `rover.exe` to its correct name. This has been corrected and extra CI and unit tests added to prevent a recurrence. - **Make sure a message is returned to the user when cloud config is updated correctly - @loshz PR #2063** - **Fix a regression in `rover dev` where it would no longer watch subgraphs correctly - @jonathanrainer PR #2065** ## 🛠 Maintenance - **Integrate the Smoke Tests Into Integration Test Framework To Allow Easier Extension - @jonathanrainer PR #1999** - **Add nicer names to GitHub actions workflow - @jonathanrainer PR #2002** - **Add test for subgraph introspect - @jonathanrainer PR #2003** - **Update node.js packages - @jonathanrainer PR #2006** Includes `eslint` to v9.8.0 and `node` to v20.16.0 - **Update Rust to v1.80.0 - @jonathanrainer PR #2007** - **Fix up CODEOWNERS to bring us inline with standard - @jonathanrainer PR #2016** - **Add E2E test for `supergraph compose` - @aaronArinder PR #2005** - **Add E2E test for `subgraph fetch` - @jonathanrainer PR #2015** - **Update Rust crates - @aaronArinder PR #2011** Includes `apollo-parser` to v0.8 and `octocrab` to v0.39.0 - **Update apollographql/router to v1.52.0 - @aaronArinder PR #2010** - **Add E2E test for `supergraph compose` - @aaronArinder PR #2005** - **Rename a test and add a `#[once]` macro to a fixture - @aaronArinder PR #2017** - **Add E2E tests for `graph introspect` - @jonathanrainer PR #2020** - **Add missing inherit for secrets - @jonathanrainer PR #2021** - **Add E2E tests for `whoami` - @jonathanrainer PR #2022** - **Update rstest to v0.22.0 - @jonathanrainer PR #2030** - **Add E2E tests for `config clear` - @aaronArinder PR #2029** - **Add E2E tests for `subgraph lint` - @aaronArinder PR #2023** - **Add E2E tests for `subgraph publish` - @jonathanrainer PR #2031** - **Add E2E tests for `graph fetch` - @aaronArinder PR #2026** - **Add E2E tests for `supergraph fetch` - @aaronArinder PR #2024** - **Add E2E tests for `subgraph list` - @aaronArinder PR #2027** - **Add E2E tests for `graph check` and `subgraph check` - @aaronArinder PR #2025** - **Add E2E tests for `install plugin` - @aaronArinder PR #2028** - **Make E2E tests account for changes in #2019 - @jonathanrainer PR #2032** - **Deprecate the use of Emoji - @loshz PR #2034** - **Let E2E tests message Slack if there are nightly failures - @jonathanrainer PR #2033** - **Tighten up Slack Messaging for E2E tests - @jonathanrainer PR #2039** - **Update `axios-mock-adapter` to v2.0.0 - @jonathanrainer PR #2043** - **Update `derive-getters` to v0.5.0 - @jonathanrainer PR #2042** - **Update `eslient` to v9.9.0 - @jonathanrainer PR #2041** - **Update Rust to v1.80.1 - @jonathanrainer PR #2040** - **Update axios to v1.7.4 - @jonathanrainer PR #2048** - **Update CODEONWERS - @aaronArinder PR #2052** - **Update termimad to v0.30.0 - @jonathanrainer PR #2054** - **Add step to fail workflow if matrix branch fails - @jonathanrainer PR #2044** - **Increase test coverage for operations/cloud/config - @loshz PR #2057** - **Update `gh` CircleCI Orb to v2.4.0 - @jonathanrainer PR #2062** - **Update `mockito` to v1.5.0 - @jonathanrainer PR #2061** - **Update `dircpy` to v0.3.19 - @jonathanrainer PR #2060** ## 📚 Documentation - **Document E2E test gotchas - @aaronArinder PR #2018** - **Fix table to be compatible with new docs platform - @shorgi PR #2038** - **Remove unhelpful note - @Meschreiber PR #2053** - **Add Summit callout - @Meschreiber PR #2058** - **Adds `--graph-ref` to supergraph compose docs - @jackonawalk PR #2037**

Geal added 30 commits March 14, 2024 16:24

temporarily make build scripts faster

f4032c4

move to an async reqwest client

9d19548

more fixes

a144232

fix issues

9d0a3fd

lint

120a17d

use futures aware channel

1b2d6e6

fix

6cc15fe

sleep

abe8dee

lint

40b35ab

backtrace

b75a531

fixes

bc95dd3

fixes

c6c4d03

fmt

73ad050

fix

f6c98d3

change back CI

d8c7d2e

Merge branch 'main' into geal/what-color-is-your-rover

20ce999

temporarily make build scripts faster

c5f68aa

move to an async reqwest client

09d409f

more fixes

3553d6d

fix issues

91c8b4d

lint

bfd3260

use futures aware channel

5f62b5b

fix

f7aaf69

sleep

f5de9e0

lint

16b0aa2

backtrace

1d9b26c

fixes

d9250e8

fixes

737473e

fmt

877fae7

fix

34d008e

aaronArinder added 5 commits July 31, 2024 16:24

fix(async): missed asyncifications during merge

ddda490

fix(merge): bad merge resolutions

a447870

appease clippy

8e650db

fix(tests): kill println

1bfc449

aaronArinder requested a review from a team as a code owner August 7, 2024 20:00

aaronArinder mentioned this pull request Aug 7, 2024

feat(rover): asyncification #1995

Closed

jonathanrainer requested changes Aug 9, 2024

View reviewed changes

Update Cargo.toml

a5b662f

Co-authored-by: Jonathan Rainer <Jonathan.rainer@apollographql.com>

dylan-apollo reviewed Aug 9, 2024

View reviewed changes

aaronArinder added 4 commits August 9, 2024 13:32

fix(bin/main): make async

655809e

fix(introspect): warnings invalidated; toggling booleans

fe13a64

fix(leader): remove unnecessary warning

c96b715

fix(deps): remove reqwest blocking feature

cb3f6c9

jonathanrainer approved these changes Aug 12, 2024

View reviewed changes

Merge branch 'main' into asyncification

51364e4

aaronArinder enabled auto-merge (squash) August 12, 2024 17:01

fix(log): bad merge

224046d

aaronArinder merged commit ee9efe1 into main Aug 12, 2024
19 checks passed

aaronArinder deleted the asyncification branch August 12, 2024 17:30

loshz mentioned this pull request Aug 14, 2024

(fix) xtask: blocking -> async #2050

Merged

jonathanrainer added the feature 🎉 new commands, flags, functionality, and improved error messages label Aug 20, 2024

jonathanrainer added this to the v0.26.0 milestone Aug 20, 2024

jonathanrainer mentioned this pull request Aug 20, 2024

release: v0.26.0 #2064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(asyncification) #2035

feat(asyncification) #2035

aaronArinder commented Aug 7, 2024 •

edited

Loading

jonathanrainer left a comment

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

aaronArinder Aug 9, 2024

jonathanrainer Aug 8, 2024

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

aaronArinder Aug 9, 2024

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

aaronArinder Aug 9, 2024

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

jonathanrainer Aug 8, 2024

aaronArinder Aug 9, 2024

dylan-apollo Aug 9, 2024

aaronArinder Aug 9, 2024

dylan-apollo Aug 9, 2024

aaronArinder Aug 9, 2024

dylan-apollo Aug 9, 2024

aaronArinder Aug 9, 2024

aaronArinder commented Aug 9, 2024

jonathanrainer left a comment

feat(asyncification) #2035

feat(asyncification) #2035

Conversation

aaronArinder commented Aug 7, 2024 • edited Loading

jonathanrainer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronArinder commented Aug 9, 2024

jonathanrainer left a comment

Choose a reason for hiding this comment

aaronArinder commented Aug 7, 2024 •

edited

Loading