Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fast server shutdown #3068

Merged
merged 4 commits into from Mar 25, 2016
Merged

Fast server shutdown #3068

merged 4 commits into from Mar 25, 2016

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Feb 29, 2016

When a server is handling persistent connections (using http keep-alive), the timeout option for server.stop() is a bit mislabeled. If just one of the persistent connections don't make any requests within the timeout duration, the server waits for the timeout even though no requests are processing. As such, it is effectively a stop delay option.

This patch tracks the request processing state of the connections, and immediately ends idle connections on server.stop(). Additionally, it signals that the server is closing on active requests (by adding connection: close to the response), and forcibly ends the connection once the response has been submitted. This means that the timeout option works as expected, and the server is much more likely to stop before the timeout is reached).

Note that actively closing these connections means that the closed server check on new requests from #2363, is no longer needed.

The first 3 patches adds additional tests that validate the current behavior, and the final patch actually implements the new behavior along with new and modified tests.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 9, 2016

This keeps failing on node v4.0.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Mar 10, 2016

OK, I will look into it.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Mar 10, 2016

My investigation shows that the test fails because wreck never receives the reply. The behavior changes in the v4.1.1 to v4.1.2 update, and I believe my test exposes the bug / security issue fixed in nodejs/node@0504066.

Do we really need to fully support 4.0.0 considering outmoded/hapi-contrib#54?

@hueniverse hueniverse self-assigned this Mar 11, 2016
@hueniverse hueniverse added this to the 13.1.1 milestone Mar 11, 2016
@hueniverse hueniverse removed this from the 13.1.1 milestone Mar 11, 2016
@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Mar 11, 2016

Look at that, my rebased version passes 😉

Let me know if you have any other concerns.

@@ -285,6 +285,11 @@ internals.transmit = function (response, callback) {
}
}

const isInjection = Shot.isInjection(request.raw.req);
if (!isInjection && !request.connection._started) {
Copy link
Contributor

@hueniverse hueniverse Mar 25, 2016

I don't like accessing non-public node apis with _started.

Copy link
Contributor Author

@kanongil kanongil Mar 25, 2016

This is a Hapi connection property lookup to know when server.stop() has been called.

Copy link
Contributor

@hueniverse hueniverse Mar 25, 2016

My bad.

@hueniverse hueniverse merged commit 0cd1b62 into hapijs:master Mar 25, 2016
1 check passed
@hueniverse hueniverse added this to the 13.2.3 milestone Mar 25, 2016
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants