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

feat(http-server): handle http keepalive connections upon application.stop() #4146

Merged
merged 3 commits into from
Nov 28, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 18, 2019

Inspired by https://github.com/godaddy/terminus and https://github.com/hunterloftis/stoppable.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng force-pushed the http-keepalive branch 2 times, most recently from 7b955b2 to 345217b Compare November 18, 2019 23:03
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition 👏

Have you considered updating our project template(s) to configure a reasonable gracePeriodForClose, for example 1 second? This will make the feature easier to discover, because developers will see it directly in code, no need to search documentation (and know what to search for). If you decide to make this change, then please put it in a new commit (or even a new PR), so that we have meaningful release notes.

Also do we need to update any pages in docs/site to mention this new flag?

@raymondfeng raymondfeng force-pushed the http-keepalive branch 3 times, most recently from 0365d87 to 69554ba Compare November 19, 2019 19:00
@raymondfeng raymondfeng requested a review from bajtos November 21, 2019 15:28
@raymondfeng raymondfeng added the CloudNative Cloud native enablement label Nov 22, 2019
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new version is much better, but not there yet.

docs/site/Server.md Outdated Show resolved Hide resolved
@raymondfeng
Copy link
Contributor Author

@bajtos Comments addressed. PTAL.

@raymondfeng raymondfeng merged commit 5a243e2 into master Nov 28, 2019
@raymondfeng raymondfeng deleted the http-keepalive branch November 28, 2019 17:45
@bajtos bajtos added the feature label Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CloudNative Cloud native enablement feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants