Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print HTTP routes in stable order #1469

Closed
itowlson opened this issue May 10, 2023 · 5 comments · Fixed by #1472
Closed

Print HTTP routes in stable order #1469

itowlson opened this issue May 10, 2023 · 5 comments · Fixed by #1472
Assignees

Comments

@itowlson
Copy link
Contributor

When Spin prints:

Available Routes:
  this: http://127.0.0.1:3000/this
  that: http://127.0.0.1:3000 (wildcard)
  other: http://127.0.0.1:3000/other

the order is currently random. This defeats muscle memory, and requires users to scan the whole list to find the one they're interested in. (You scoff? Try it on https://github.com/fermyon/developer/.)

The order should:

  • At minimum, be stable: not vary from run to run
  • Ideally, lend itself to searching, e.g. alphabetical, or most general prefix first and alphabetically by URL within that

I have a feeling it used to be stable and the randomness is a regression - I only started noticing this on the docs site recently but I could be wrong!

@itowlson
Copy link
Contributor Author

Looks like this was introduced sometime between Spin 1.0 and 1.1. 1.0 appears to use the order of components in the spin.toml file (which is fine). 1.1 appears to have the random order.

@itowlson
Copy link
Contributor Author

Okay this is the weirdest thing. It looks like the randomness started when #1321 got merged. But that's all about the Wasm component model, not anything to do with how we store Spin component metadata and HTTP routes, so seems unlikely!

@dicej I am sorry to bug you with what I worry is a false positive, but can you think of any way in which the CM work could have had this effect?

@rylev
Copy link
Collaborator

rylev commented May 10, 2023

We're using an IndexMap to store routes which has a stable iteration order (i.e., it always iterates in the order items are added). So it seems like the issue is likely caused by how we're populating those routes. It looks like ultimately this comes down to how triggers are deserialized as part of a LockedApp. It would be interesting to test whether locked apps are being serialized in different ways or if the deserialization is not guaranteed to always result in the same order.

@dicej dicej self-assigned this May 10, 2023
@dicej
Copy link
Contributor

dicej commented May 10, 2023

Yes, this was introduced as part of my component work. The fix is pretty simple:

diff --git a/crates/trigger/src/lib.rs b/crates/trigger/src/lib.rs
index 175d6f9..4023244 100644
--- a/crates/trigger/src/lib.rs
+++ b/crates/trigger/src/lib.rs
@@ -8,6 +8,7 @@ use std::{collections::HashMap, marker::PhantomData, path::PathBuf};
 
 use anyhow::{anyhow, Context, Result};
 pub use async_trait::async_trait;
+use indexmap::IndexMap;
 use serde::de::DeserializeOwned;
 
 use spin_app::{App, AppComponent, AppLoader, AppTrigger, Loader, OwnedApp};
@@ -194,7 +195,7 @@ impl<Executor: TriggerExecutor> TriggerAppEngine<Executor> {
                     })?,
                 ))
             })
-            .collect::<Result<HashMap<_, _>>>()?;
+            .collect::<Result<IndexMap<_, _>>>()?;
 
         let mut component_instance_pres = HashMap::default();
         for component in app.borrowed().components() {

PR coming shortly.

@itowlson
Copy link
Contributor Author

@dicej Oh, awesome. Thank you!

dicej added a commit to dicej/spin that referenced this issue May 10, 2023
This regressed due to changes I made to `TriggerAppEngine::new` to
support components.  I used a `HashMap` without realizing the
iteration order of triggers (and their configs) matters.

Fixes fermyon#1469

Signed-off-by: Joel Dice <[email protected]>
dicej added a commit that referenced this issue May 11, 2023
This regressed due to changes I made to `TriggerAppEngine::new` to
support components.  I used a `HashMap` without realizing the
iteration order of triggers (and their configs) matters.

Fixes #1469

Signed-off-by: Joel Dice <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants