Avoid race condition between HttpServer.stop() and HttpServerSetup methods#64487
Avoid race condition between HttpServer.stop() and HttpServerSetup methods#64487joshdover merged 1 commit intoelastic:masterfrom
Conversation
src/core/server/http/http_server.ts
Outdated
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
Good idea, I will try that tomorrow.
There was a problem hiding this comment.
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.
d54fec9 to
ea4cb3f
Compare
|
@pgayvallet @restrry I have changed this to use Pierre's suggestion and wait for start to complete before stopping continues. |
|
Pinging @elastic/kibana-platform (Team:Platform) |
ea4cb3f to
e34ae40
Compare
|
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. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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
stopat 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 duringsetupto insert thestopcall at the specific point that could cause failure.Checklist
Delete any items that are not applicable to this PR.
For maintainers