fix(cargo-shuttle): prompt for new port if port is taken

[BadgerBloke]

Description of change

cargo shuttle run panic when the default port i.e., 8000 was already in use.
This fix is checking that port 8000 is available or not?
If it finds 8000 free then it continues with earlier implementation but if it found the as not free then it is checking for available 2 consecutive ports using find_available_port function and find_available_port function returns the higher one. So that, rest of the implementation like runtime::start will work as expected.

runtime::start(
            service.is_wasm,
            Environment::Local,
            &format!("http://localhost:{provisioner_port}"),
            None,
            run_args.port - idx - 1,
            runtime_executable,
            service.workspace_path.as_path(),
        )

The ports range has been set as default port - 1 to 65535 i.e., 7999 to 65535

Closes #1259

How has this been tested? (if applicable)

Test performed by spawning multiple instances of shuttle one after another and all of them was able to run properly on newer ports.

[orhun]

Overall looks good to me, just had some comments. Let's see what other maintainers think.

[jonaro00]

Looking pretty good now!
Writing a test for this is probably a bit cumbersome. I want to do some manual testing when it's ready anyways.
In the mean time, you could take a look at the task of timing out the confirmation if there is no response within, say, 10 seconds. In that case, we could exit(1) actually.

[BadgerBloke]

Looking pretty good now!

👍🏻

Writing a test for this is probably a bit cumbersome. I want to do some manual testing when it's ready anyways.

👍🏻

In the mean time, you could take a look at the task of timing out the confirmation if there is no response within, say, 10 seconds. In that case, we could exit(1) actually.

Sure, but instead of exit(1) can we continue with suggested port?

[jonaro00]

Yeah, we can do that. We'll see what others think as well.

[oddgrd]

LGTM, thanks!

[BadgerBloke]

LGTM, thanks!

Thanks 😊, I'm trying for the timeout feature but that doesn't seems easy one, I'll be able to invest more time on weekend

[jonaro00]

I double checked, and we do need to send in services.len() so that it can check N adjacent port numbers.
So, if there is only one service, we do like we do now. If there are two, we need to check base and base+1. If any of those were taken, check base+10 and base+11 etc. Do you have time to work on this this week @BadgerBloke? I can jump in and help if needed.

[BadgerBloke]

Sure @jonaro00 , I'll work and complete this port checking at least.

Since you have already provided lots of info so it seems easily doable.

However I'm facing challenge for the timeout feature. If you have some time then please share something.

[jonaro00]

The timeout is not needed to complete this PR. I might take a look at it if I have time.

[jonaro00]

@BadgerBloke How is progress on this? We aim to get this into a release this coming week, so I might hop in and finish any remaining work unless you are already on it ;)

[BadgerBloke]

Yes @jonaro00 I'm on it and push the changes by today itself.

[BadgerBloke]

@jonaro00 I think just by adding && portpicker::is_free_tcp(port + 1) inside for loop we will be able to achieve that. Do we still have to use services.len().

Please let me know if I'm still missing something

if portpicker::is_free_tcp(port) && portpicker::is_free_tcp(port + 1) {
    available_port = port;
    break;
}

[jonaro00]

Checking port+1 would not be enough if N > 2.
Inside the loop that takes steps of 10, there needs to be an inner loop that checks if all N adjacent ports starting from 8000/8010/etc are free.

So, if there are 3 services, and port 8000 is taken, it checks ports 8010, 8011, and 8012 in the next loop iteration.

There can be issues with services not stopping and remaining open on port 8000. This is one of the reasons we are adding this feature.

[BadgerBloke]

Ok, i think now I got the point, let me make the adjustment

[jonaro00]

Some comments

[BadgerBloke]

Some comments

Thanks, let me fix that

[jonaro00]

Nice job!
I tested it locally, and while it is not bulletproof yet, I think this is good for merging now.

[BadgerBloke]

Nice job!
I tested it locally, and while it is not bulletproof yet, I think this is good for merging now.

Thanks 😊

I've noted the timeout feature, please let me know other points I'll look into that

[jonaro00]

We might not need it. For example, we could just make it take the new ports without asking, but print a warning about it instead.
Which one would you prefer as a user?
I will also ask what others think.

[BadgerBloke]

It sounds really good to me. There isn't much difference from UX point of view. In case do not want to run on suggested port then I can stop the started service. So, it's completely fine for me.

Just a rough comparision:

Ability	With Timeout ⏳	With Warning Print ⚠️
Continue on Suggested port	After timeout	Immediately
Cancel the process on suggested port	Immediately	Hard close after starting ctrl + c
Start on custom specific port	after cancelling (immediately)*	after stopping the started service*

*provided we supports -p custom_port or --port custom_port arg

[oddgrd]

Looks good to me! I'm curious, why do we need a timeout here?

[jonaro00]

This was the idea behind it. Not strictly necessary ofc. #1270 (review)

[BadgerBloke]

Thank you so much guys 😍
It was a great learning for me