Fix API Address Collisions: expose listener for serving

[rMaxiQp]

What?

Expose listeners that getFreeBindAddr() creates

Why?

Currently, getFreeBindAddr() finds a free-to-use address by try-binding. After it finds the valid port, it returns the API address, and closes the listener. In between the old listener closing and new listener opening, the address port can be stolen. One fix here would be exposing the old listener without closing.

One caveat, now the GlobalState has a net.Listener field hanging mainly because it's needed for testing... Maybe there can be a better approach here.

Checklist

• [x] I have performed a self-review of my code.
• [x] I have commented on my code, particularly in hard-to-understand areas.
• [ ] I have added tests for my changes.
• [x] I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

• [ ] I have added the correct milestone and labels to the PR.
• [ ] I have updated the release notes: link
• [ ] I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
• [ ] I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

Relates to #3846

[AgnesToulet]

Hi! Thanks for your contribution, the approach seems OK to me. An alternative would be to move the code responsible for setting up the server in the api package (along the GetServer function) and mock it in the tests but it also seems a big refactor "just" for testing.

To make the code more readable, I would move theses lines in a separate function though and I would completely separate the code used in tests (where c.gs.ServerListener != nil) and the code that existed before with comments to explain all this. Something like:

// ServerListener is set up in tests 
if c.gs.ServerListener != nil {
	if aerr := srv.Serve(c.gs.ServerListener); aerr != nil {
		logger.WithError(aerr).Error("Error from API server")
		c.gs.OSExit(int(exitcodes.CannotStartRESTAPI))
	}
} else {
	if aerr := srv.ListenAndServe(); aerr != nil && !errors.Is(aerr, http.ErrServerClosed) {
		// Only exit k6 if the user has explicitly set the REST API address
		if cmd.Flags().Lookup("address").Changed {
			logger.WithError(aerr).Error("Error from API server")
			c.gs.OSExit(int(exitcodes.CannotStartRESTAPI))
		} else {
			logger.WithError(aerr).Warn("Error from API server")
		}
	}
}

This ensures the behavior at runtime doesn't change (as in your changes, where the error handling is a bit different than it was before).

Also, the xk6 tests are failing on Windows and MacOS with your changes. I'm not sure why but I can reproduce on my Windows machine so I'll look into it deeper today.

[rMaxiQp]

Hi @AgnesToulet , thank you for your input! I tried to refactor the code a bit to address your comment. Failing on Mac and Windows is strange... I was trying to reproduce the error on my Mac Air, and it seems to struggle with other issues atm (many browser instances are created). I'll try to reproduce on Mac a bit more later but also create a Windows setup.

[rMaxiQp]

Hi @AgnesToulet , thank you for the reviews!

I didn't realize that my change altered the default behavior earlier 🙈. Thank you for pointing it out. Though it's a bit verbose, now we should have same behavior between actual and test besides the net.Listener change.

[rMaxiQp]

Hi @codebien, thank you for the comments! I can start working on migrating the refactor on top a separate PR. For other comments, I shared my current understanding and hopefully it helps to complete the picture. If there are some context that I'm missing out, please let me know!