adding custom port in cli opt-in #3182

seun-ja · 2025-07-03T15:38:34Z

Aims to resolve issue #2470

Custom port can be provided in CLI, optional though

seun-ja · 2025-07-03T15:42:10Z

I came across this error after running make test.

error: 1 target failed:
    `-p spin-factor-outbound-http --test factor_test`
make: *** [test-unit] Error 101

All tests passed. So I don't know it this is something that can be ignored

lann · 2025-07-03T19:37:36Z

@seun-ja Could you please take a look at https://spinframework.dev/v3/contributing-spin#committing-and-pushing-your-changes? This project requires commit signing and sign-off (two entirely separate things).

itowlson

I'm not sure this addresses #2470 after all - that issue asks for Spin to automatically find a free port number (which we are dubious about wanting as default), whereas this PR provides another way to select a fixed port number.

Perhaps it would be useful to say more about the motivation for this change?

itowlson · 2025-07-03T20:10:26Z

crates/trigger-http/src/lib.rs

+
+    /// The port to listen on
+    #[clap(long = "port", env = "SPIN_HTTP_LISTEN_PORT")]
+    pub port: Option<u16>,


Could you expand on how you see this interacting with --listen? The way I usually override the port is e.g. spin up --listen 127.0.0.1:4000 - is this envisaged as a shortcut to that? Or a way to set the address and port separately?

itowlson · 2025-07-03T20:15:34Z

crates/trigger-http/src/server.rs

+
        if let Some(tls_config) = self.tls_config.clone() {
-            self.serve_https(listener, tls_config).await?;
+            self.serve_https(listener?, tls_config).await?;


I'm not sure I love having to ? the listener everywhere it's used. Why not stick with the previous pattern of failing errors before assigning the listener variable?

seun-ja · 2025-07-03T21:29:00Z

In hindsight, this isn't an ideal approach.

I didn't take into consideration that --listen is already custom. All I need to do here is just check for the error kind that I already used in the last commit, check if it is AddrInUse and use a wildcard 0 so the OS can just assign one.

itowlson · 2025-07-03T21:34:18Z

@seun-ja Makes sense! It might be worth reading through the comment thread on that issue though, if you haven't already, as there were some doubts/discussions - e.g. I know @lann was reluctant to have this behaviour by default. Thanks for looking into it!

lann · 2025-07-03T22:03:45Z

IMO if you'd be happy with a random port then you should just listen on :0 to begin with, but then again I probably won't be using this feature anyway. 🙂

Based on the discussion in the issue I'd suggest something like this:

Add a new --find-free-port flag
If bind fails with AddrInUse and the flag was set, add 1 to the port and try again
Fail after 10 tries

karthik2804 · 2025-07-04T06:22:00Z

Would be a good idea to prompt the user if we are on an interactive CLI where we can have a confirm prompt with the user if they want to use a different port?

itowlson · 2025-07-06T21:01:42Z

@karthik2804 Because multi-trigger-type applications exist, I'd be inclined to adopt a philosophy that triggers should not prompt for input. Otherwise we risk having to manage several child processes all prompting at once, which is likely to be a bit chaotic. Or partial starts where one trigger is processing events but another is stuck at a prompt. (Sure, we could do a little dance to inform a trigger that it's the only one so it's allowed to prompt, but this makes both up and the trigger more complicated, and means the trigger behaves differently depending on single vs multiple.) Open to thoughts of course but for now I'd suggest we stick to a helpful failure message.

itowlson

Some style and wording nits, but my main concern is if the loop is fully doing what you intend. That may be a code reading fail on my part though! Great to see this progressing!

crates/trigger-http/src/lib.rs

itowlson · 2025-07-06T21:07:22Z

crates/trigger-http/src/server.rs

+                            }
+                            Err(err) => {
+                                if err.kind() == ErrorKind::AddrInUse {
+                                    continue;


This routine gets into some quite deep nesting (a match inside a loop inside an if inside a match) and for me it was hard to follow. Is it feasible to break out a helper function?

There should be.. would look into it

I think there's still value in breaking out a helper. The long and complicated if and loop make it very hard (at least for me) to see what is basically simple "if it succeeds, use it; if it doesn't, search or bail." Consider e.g.

let listener = match TcpListener::bind(self.listen_addr).await { Ok(listener) => listener, Err(err) if self.find_free_port && err.kind() == ErrorKind::AddrInUse => self.search_for_free_port().await?, Err(err) => anyhow::bail!(...) } fn search_for_free_port(&self) -> anyhow::Result<TcpListener> { // declare mutables // loop // ok or error }

There's probably more breaking out you could do but to me this greatly clarifies the top-level logic, and gives a meaningful name to that long chunk for which I currently have to infer the intention from the code.

Yeah I've never found a nice way to do retry loops without a separate function so you can return from the middle of the loop.

itowlson · 2025-07-06T21:09:26Z

crates/trigger-http/src/server.rs

+                                if err.kind() == ErrorKind::AddrInUse {
+                                    continue;
+                                }
+                                return Err(anyhow::anyhow!("Unable to listen on {}", addr));


If there's a non-AddrInUse error on one of the scanned ports, this will produce a strange error message e.g. the user said to listen on port 4000 and gets an error about port 4007.

itowlson · 2025-07-06T21:11:16Z

crates/trigger-http/src/server.rs

+                                if err.kind() == ErrorKind::AddrInUse {
+                                    continue;
+                                }
+                                return Err(anyhow::anyhow!("Unable to listen on {}", addr));


Is it correct that we bail here? Why would an error on port 3002 stop us from trying port 3003? (Is it, perhaps, that errors other than AddrInUse imply we've gone to a bad place?)

It only bails if the error kind isn't AddrInUse, so it doesn't stop us from trying if 3002 isn't available.

I understand that it continues on AddrInUse. I am asking why it bails on other errors. Is the assumption that other errors are unrecoverable and there is no point searching further?

There are probably other recoverable errors. I only focused on AddrInUse for the scope of this issue.

Should I add the ones that could be recoverable here, or just create a new issue for that?

If it's going to turn into a faff then don't worry about it - stick with what you have and we can always re-evaluate if something comes up (i.e. no need to raise an issue either).

crates/trigger-http/src/server.rs

itowlson · 2025-07-06T21:16:10Z

crates/trigger-http/src/server.rs

+                if self.find_free_port && err.kind() == ErrorKind::AddrInUse {
+                    let mut found_listener = None;
+                    for _ in 1..=9 {
+                        let mut addr = self.listen_addr;


This reads as if you are resetting addr each time through the loop, which means that each iteration is trying self.listen_addr with +1 to its port. Have you confirmed that if e.g. 3000-3002 are all in use then this does find 3003? I may be misreading for sure

itowlson · 2025-07-06T21:17:43Z

crates/trigger-http/src/server.rs

+                        }
+                    }
+                } else {
+                    return Err(anyhow::anyhow!("Unable to listen on {}", self.listen_addr));


Consider mentioning the --find-free-port option e.g.

Suggested change

return Err(anyhow::anyhow!("Unable to listen on {}", self.listen_addr));

return Err(anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr));

itowlson · 2025-07-06T21:18:54Z

tests/testing-framework/src/runtimes/in_process_spin.rs


    let app = spin_app::App::new("my-app", locked_app);
-    let trigger = HttpTrigger::new(&app, "127.0.0.1:80".parse().unwrap(), None)?;
+    let trigger = HttpTrigger::new(&app, "127.0.0.1:80".parse().unwrap(), None, true)?;


Do we want to end up on a random port here? I am not sure if the testing infra will be happy with that. Although I guess it should never happen. But it might be better for it to fail here if it did.

itowlson · 2025-07-06T21:22:09Z

crates/trigger-http/src/server.rs

+                        Some(listener) => listener,
+                        None => {
+                            return Err(anyhow::anyhow!(
+                                "All retries failed. Unable to bind to a free port"


From the user point of view, they told us to find a free port, not to "retry" anything, so I'd suggest we re-word this error message in those terms e.g.

Suggested change

"All retries failed. Unable to bind to a free port"

"Couldn't find a free port in the range {min}-{max}. Consider retrying with a different base port."

or something like that?

itowlson · 2025-07-06T21:24:41Z

crates/trigger-http/src/server.rs

+                        }
+                    }
+
+                    match found_listener {


You can achieve the same effect with found_listener.ok_or_else(...)? - your call which you think is more readable!

itowlson · 2025-07-07T20:40:20Z

crates/trigger-http/src/server.rs

+                    for _ in 1..=9 {
+                        addr.set_port(addr.port() + 1);
+
+                        if !addr.port() == u16::MAX {


The ! operator here is bitwise complement. Is that what you intended?

I missed that, was initially trying a while loop. Would fix and push a fix now

itowlson

Thanks for all the updates! I think the logic is sound (except for one nitty detail of an error message which is not a blocker), but I remain really troubled by the long nameless block containing the search logic. It is long enough and nested enough that I find it really hard to be confident about following the logic, and in particular to visually match open and close braces to know what scope any given line is in, especially towards the end of the block. I've suggested a way that seems a lot clearer to me (and that, I think, helps with the nitty error thing), but I'm not wedded to it, and it doesn't fully solve the problem: if you have better ideas then go for it. But I do think it would be desirable to do something unless it turns out impractical. Which it sometimes does, so please push back if it's going to be a big pain!

itowlson · 2025-07-09T20:07:42Z

crates/trigger-http/src/server.rs

+                            }
+                            Err(err) => {
+                                if err.kind() == ErrorKind::AddrInUse {
+                                    continue;


I think there's still value in breaking out a helper. The long and complicated if and loop make it very hard (at least for me) to see what is basically simple "if it succeeds, use it; if it doesn't, search or bail." Consider e.g.

let listener = match TcpListener::bind(self.listen_addr).await { Ok(listener) => listener, Err(err) if self.find_free_port && err.kind() == ErrorKind::AddrInUse => self.search_for_free_port().await?, Err(err) => anyhow::bail!(...) } fn search_for_free_port(&self) -> anyhow::Result<TcpListener> { // declare mutables // loop // ok or error }

There's probably more breaking out you could do but to me this greatly clarifies the top-level logic, and gives a meaningful name to that long chunk for which I currently have to infer the intention from the code.

itowlson · 2025-07-09T20:18:49Z

crates/trigger-http/src/server.rs

+                    found_listener.ok_or_else(|| anyhow::anyhow!(
+                        "Couldn't find a free port in the range {}-{}. Consider retrying with a different base port.",
+                        self.listen_addr.port(),
+                        self.listen_addr.port() + 9


Duplicate magic number (with line 157). Make this a const, or if you adopt the helper function, it could even be a parameter passed to the helper.

itowlson · 2025-07-09T20:20:45Z

crates/trigger-http/src/server.rs

+                        self.listen_addr.port() + 9
+                    ))?
+                } else {
+                    anyhow::bail!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)


It occurs to me this can happen even if they passed --find-free-port (if the error was something other than AddrInUse) (at least I think it can, I'm finding it tricky to visually match which if or case we're in here). So we probably need to distinguish those cases I'm afraid - sorry for the bad steer.

Also, we should surface what the actual error is, because there are few things more maddening than being told
"computer says no" and not being told why.

If you decide to go with my suggestion about breaking out the loop logic, this might be fairly easy e.g.

match TcpListener::bind(self.listen_addr).await { Ok(listener) => listener, Err(err) if err.kind() == ErrorKind::AddrInUse && self.find_free_port => self.search_for_free_port().await?, Err(err) if err.kind() == ErrorKind::AddrInUse => anyhow::bail!(/* use --find-free-port */), Err(err) => anyhow::bail!(/* --find-free-port wouldn't have helped, just report the error */) }

but not sure, would need some testing and another pair of eyes for sure!

I think it might be easier to follow if the find_free_port check moved to the outside, e.g.

let listener = if self.find_free_port { // try 10 times here } else { // only try once here TcpListener::bind(self.listen_addr)... };

That would make it easier to have the error messages make sense in the two different cases.

lann · 2025-07-09T21:24:17Z

crates/trigger-http/src/server.rs

+                            Err(err) => {
+                                if err.kind() == ErrorKind::AddrInUse {
+                                    continue;
+                                }
+                                anyhow::bail!("Unable to listen on {}", self.listen_addr);
+                            }


It's a little confusing to use the original port in this error message and we should include the source error.

Separately/optionally, you could move the if/continue out to a match guard clause:

Suggested change

Err(err) => {

if err.kind() == ErrorKind::AddrInUse {

continue;

}

anyhow::bail!("Unable to listen on {}", self.listen_addr);

}

Err(err) if err.kind() == ErrorKind::AddrInUse => {

// Try the next port

}

Err(err) => {

anyhow::bail!("Unable to listen on {addr}: {err:?}");

}

itowlson · 2025-07-10T01:52:28Z

crates/trigger-http/src/server.rs

+                if err.kind() == ErrorKind::AddrInUse {
+                    anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)
+                } else {
+                    anyhow::anyhow!("Unable to listen on {}", self.listen_addr)


I still feel like we are dropping the actual error here - am I missing something?

itowlson · 2025-07-10T01:56:10Z

crates/trigger-http/src/server.rs

+                );
+            }
+
+            addr.set_port(addr.port() + 1);


I think the ordering here will cause us to start searching at requested port plus 1, rather than at the requested port, even if the requested port is free. Am I misreading?

itowlson · 2025-07-10T02:12:22Z

crates/trigger-http/src/server.rs

+        } else {
+            TcpListener::bind(self.listen_addr).await.map_err(|err| {
+                if err.kind() == ErrorKind::AddrInUse {
+                    anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)


It occurs to me that since we now know the kind of error we can reword this to e.g.

Suggested change

anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)

anyhow::anyhow!("{} is already in use. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)

itowlson

Thanks for persevering with this! Looks good!

Signed-off-by: Aminu 'Seun Joshua <[email protected]> Signed-off-by: Aminu 'Seun Joshua <[email protected]> approach revamp using --find-free-port flag Signed-off-by: Aminu 'Seun Joshua <[email protected]> Signed-off-by: Aminu 'Seun Joshua <[email protected]> improvement: readability and refactor Signed-off-by: Aminu 'Seun Joshua <[email protected]> using bail instead Signed-off-by: Aminu 'Seun Joshua <[email protected]> fix: bitwise logic error Signed-off-by: Aminu 'Seun Joshua <[email protected]> refactor Signed-off-by: Aminu 'Seun Joshua <[email protected]> refactor: cleaner logic Signed-off-by: Aminu 'Seun Joshua <[email protected]> refactor: re-wording + logic Signed-off-by: Aminu 'Seun Joshua <[email protected]>

seun-ja force-pushed the opt-in-custom-port branch 2 times, most recently from 07d6d80 to f9d5295 Compare July 3, 2025 20:05

itowlson reviewed Jul 3, 2025

View reviewed changes

seun-ja force-pushed the opt-in-custom-port branch from fbed697 to f2b9734 Compare July 4, 2025 15:03

itowlson reviewed Jul 6, 2025

View reviewed changes

itowlson reviewed Jul 7, 2025

View reviewed changes

seun-ja requested a review from itowlson July 9, 2025 09:01

itowlson requested changes Jul 9, 2025

View reviewed changes

lann requested changes Jul 9, 2025

View reviewed changes

seun-ja requested a review from itowlson July 10, 2025 00:21

itowlson reviewed Jul 10, 2025

View reviewed changes

seun-ja requested a review from itowlson July 10, 2025 08:15

itowlson approved these changes Jul 10, 2025

View reviewed changes

itowlson requested a review from lann July 10, 2025 20:07

lann approved these changes Jul 10, 2025

View reviewed changes

seun-ja force-pushed the opt-in-custom-port branch from c589818 to 3f9c2ae Compare July 10, 2025 21:00

seun-ja force-pushed the opt-in-custom-port branch from 3f9c2ae to dfbef34 Compare July 10, 2025 21:35

itowlson enabled auto-merge July 10, 2025 21:39

itowlson merged commit cb47654 into spinframework:main Jul 10, 2025
17 checks passed

seun-ja deleted the opt-in-custom-port branch July 10, 2025 22:18

itowlson mentioned this pull request Jul 14, 2025

Hardcoded default port numbers prevent concurrent execution - consider testing if port is in use #2470

Closed

rancherbot mentioned this pull request Aug 27, 2025

rddepman: bump spinCLI from 3.3.1 to 3.4.0 rancher-sandbox/rancher-desktop#9163

Merged

	return Err(anyhow::anyhow!("Unable to listen on {}", self.listen_addr));
	return Err(anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr));

	"All retries failed. Unable to bind to a free port"
	"Couldn't find a free port in the range {min}-{max}. Consider retrying with a different base port."

	anyhow::anyhow!("Unable to listen on {}. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)
	anyhow::anyhow!("{} is already in use. To have Spin search for a free port, use the --find-free-port option.", self.listen_addr)

adding custom port in cli opt-in #3182

adding custom port in cli opt-in #3182

Uh oh!

Conversation

seun-ja commented Jul 3, 2025

Uh oh!

seun-ja commented Jul 3, 2025

Uh oh!

lann commented Jul 3, 2025

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seun-ja commented Jul 3, 2025

Uh oh!

itowlson commented Jul 3, 2025

Uh oh!

lann commented Jul 3, 2025

Uh oh!

karthik2804 commented Jul 4, 2025

Uh oh!

itowlson commented Jul 6, 2025

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lann Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lann Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

lann Jul 9, 2025 •

edited

Loading

lann Jul 9, 2025 •

edited

Loading