Avoid race condition between HttpServer.stop() and HttpServerSetup methods

[joshdover]

Summary

Fixes #64381
Fixes #64480

This change should reduce flakiness in integration tests. The issue that appeared to be causing other integration tests to fail was a situation where the server would start stopping before setup had completed, causing some HttpServiceSetup methods to fail.

I attempted writing a high-level test for executing stop at different points in the setup lifecycle, but I was unable to find a practical way of doing so that wasn't highly specific the one known scenario. The challenge is in controlling the execution of the various promises that are resolved during setup to insert the stop call at the specific point that could cause failure.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility, including a check against IE11

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[pgayvallet]

Would it be too pragmatic to have stop await a promise that would be resolved at the end of start?
Are there practical use cases where stop should be called before setup/start are completed (if they were invoked).

In most graceful shutdown implementations I'm aware of, this is what is actually done (either this or signal-based cancelation request/awaiting, which is definitely cleaner but also approximatively 42 times harder to implement correctly).

This should probably be done in both core's main Server and I guess HttpServer for ITs.

setup() {
  this.startingPromise = new Promise();
  // do setup
}

start() {
   // do start
   this.startingPromise.resolve(); // yea I know, this doesn't work like that. You get the idea.
}

stop() {
   if(this.startingPromise) {
      await this.startingPromise()
   }
  // do stop
}

[joshdover]

Good idea, I will try that tomorrow.

[mshustov]

I suspect there will be a lot of complaints that Kibana doesn't react to the stop signal and continue working. But 👍 as a temporary solution.

[spalger]

LGTM

[joshdover]

@pgayvallet @restrry I have changed this to use Pierre's suggestion and wait for start to complete before stopping continues.

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[joshdover]

This other solution is going to take more time than I have right now to work on this. Rolling back to the previous solution and will merge once CI is green.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: e34ae40

History

• 💚 Build #43262 succeeded d54fec91cd40159d25fc1f33a89433c89a775468

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream