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(medusa,medusa-core-utils): graceful shutdown server #3408

Merged
merged 11 commits into from
Mar 10, 2023

Conversation

carlos-r-l-rodrigues
Copy link
Contributor

What:
Helper function to gracefully shutdown the http server.

How:
All pending requests are completed before exiting the application.

Fixes: CORE-1142

@changeset-bot
Copy link

changeset-bot bot commented Mar 7, 2023

🦋 Changeset detected

Latest commit: b2b5477

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 33 packages
Name Type
medusa-core-utils Minor
medusa-react Major
@medusajs/medusa Major
medusa-interfaces Major
@medusajs/admin Major
@medusajs/inventory Major
@medusajs/medusa-js Major
medusa-payment-stripe Major
medusa-plugin-restock-notification Major
@medusajs/stock-location Major
medusa-file-minio Major
medusa-file-s3 Major
medusa-file-spaces Major
medusa-fulfillment-manual Major
medusa-fulfillment-webshipper Major
medusa-payment-adyen Major
medusa-payment-klarna Major
medusa-payment-manual Major
medusa-payment-paypal Major
medusa-plugin-algolia Major
medusa-plugin-brightpearl Major
medusa-plugin-contentful Major
medusa-plugin-discount-generator Major
medusa-plugin-economic Major
medusa-plugin-ip-lookup Major
medusa-plugin-mailchimp Major
medusa-plugin-meilisearch Major
medusa-plugin-segment Major
medusa-plugin-sendgrid Major
medusa-plugin-slack-notification Major
medusa-plugin-twilio-sms Major
medusa-plugin-wishlist Major
medusa-source-shopify Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Mar 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
staging ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 11:01AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
medusa-docs ⬜️ Ignored (Inspect) Mar 10, 2023 at 11:01AM (UTC)

@@ -1,4 +1,3 @@
import e from "express"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self-review: imported by mistake

Comment on lines -38 to -40
"@storybook/addon-contexts": "^5.3.21",
"@storybook/addon-essentials": "^6.3.12",
"@storybook/addon-info": "^5.3.21",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self-review: these 2 dependencies are not being used and preventing yarn install to work on Windows

Copy link
Contributor

@patrick-medusajs patrick-medusajs left a comment

Choose a reason for hiding this comment

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

LGTM
Would be nice to have tests to validate timeout feature and multiple connections handling work as expected.

@carlos-r-l-rodrigues
Copy link
Contributor Author

LGTM Would be nice to have tests to validate timeout feature and multiple connections handling work as expected.

I've added a few tests covering all these timeouts behavior. Thanks.

socket._connectionId = connectionId
allSockets[connectionId] = socket

socket.on("close", () => {
Copy link
Member

Choose a reason for hiding this comment

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

question: is the close always called after a end, timeout event as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. And when it's using keepAlive, it will respect server.keepAliveTimeout to close idle connections with clients.
https://nodejs.org/api/net.html#event-close_1

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM, just have one question 💪

@carlos-r-l-rodrigues carlos-r-l-rodrigues merged commit 54dcc18 into develop Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants