fix(http): remove unnecessary delay when closing server #2732
Conversation
|
|
http/server.ts
Outdated
| acceptBackoffDelay = MAX_ACCEPT_BACKOFF_DELAY; | ||
| } | ||
|
|
||
| await delay(acceptBackoffDelay); |
There was a problem hiding this comment.
Isn't the following sequence still possible?
- Server is accepting traffic.
- Server enters
delay(). - Server is closed while
delay()is still executing. - Test runner reports leaked op.
I think this could be an improvement, but I don't know that it solves the problem.
There was a problem hiding this comment.
Agreed, this should be solvable as well by aborting the delay when closing the server. I gave it a go here. However, I think there is room to improve the test case to not have it depend on the leaking detection AssertionError. Any suggestions?
http/server.ts
Outdated
| @@ -253,6 +254,8 @@ export class Server { | |||
|
|
|||
| this.#closed = true; | |||
|
|
|||
| this.#acceptBackoffDelayAbortController.abort(); | |||
There was a problem hiding this comment.
Does it work to do this after the for...of loop a couple lines down?
There was a problem hiding this comment.
I made the change here: d749829
http/server.ts
Outdated
|
|
||
| if (acceptBackoffDelay >= MAX_ACCEPT_BACKOFF_DELAY) { | ||
| acceptBackoffDelay = MAX_ACCEPT_BACKOFF_DELAY; | ||
| if (!this.#closed) { |
There was a problem hiding this comment.
I think this change can be reverted with the addition of the abort controller and my previous comment about moving the abort() down.
There was a problem hiding this comment.
Aha, that makes sense.
There was a problem hiding this comment.
Made the change here: e65d8bc
http/server.ts
Outdated
| await delay(acceptBackoffDelay, { | ||
| signal: this.#acceptBackoffDelayAbortController.signal, | ||
| }).catch((err: unknown) => { |
There was a problem hiding this comment.
Can you avoid mixing async/await with Promise APIs. In other words, make this a try...catch.
There was a problem hiding this comment.
Yes, that's reasonable. I made the change here: 4f7ef9c
| const handler = () => new Response(); | ||
| const server = new Server({ port, hostname, handler }); | ||
| server.listenAndServe(); | ||
| server.close(); |
There was a problem hiding this comment.
No need to make the change in this PR if this fixes the original issue, but I still think it would be helpful to be able to await on server.close().
There was a problem hiding this comment.
In general I think it makes sense, specially if we want to wait until all the pending handlers are resolved. However, I'm not yet comfortable enough with the codebase to make this change and I would prefer to keep it separate.
KhaledSakr
requested review from
cjihrig
and removed request for
kt3k and
bartlomieju
This conditionally removes the backoff delay when an error happens while accepting connections, if the server is already closed. This fixes the leaking async op encountered in the issue below, that can also be reproduced using the test case added in this PR.
closes #2717