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

Enable graceful shutdown for https server #85

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

erka
Copy link
Contributor

@erka erka commented Dec 14, 2024

For some reason, the httpsServer does not shut down when server.Stop is called.

erka and others added 2 commits December 15, 2024 00:11
For some reason, the httpsServer does not shut down when server.Stop is called.
@kevinmcconnell
Copy link
Collaborator

Hey @erka, thanks for fixing this! (And sorry for the slow response on the PR, been busy with the holidays and so on :))

What you had looks good, but I also thought we could extract a helper for the WaitGroup behavior, to clarify that section of the code even more. I've pushed a commit with that idea. What do you think?

I also left out the extra logging there -- mainly for simplicity, but also because the likely error we'd see there would be the context timeout any time we have to forcibly shutdown a long request, but that's not really an error; it's just part of how the shutdown operates. We could catch and swallow those errors specifically, but I'm not sure it's worth it.

@erka
Copy link
Contributor Author

erka commented Jan 3, 2025

Great refactoring @kevinmcconnell

Logs could provide valuable insights. If a forced shutdown occurs (context timeout), some users/services might not receive their data, and I personally prefer to be aware of such situations. But I also agree with you that it could be a noise in many situations.

@kevinmcconnell
Copy link
Collaborator

That's true, yes. I was mostly concerned with that timeout message suggesting that something is failing, when really the shutdown process is working as intended. But I agree it could be nice to know that connections had to be terminated.

I've made a small change so now we'll show a clear warning if that happens ("Closing active connections").

Thanks again for your contribution on this, @erka!

@kevinmcconnell kevinmcconnell merged commit 202f50a into basecamp:main Jan 6, 2025
1 check passed
@erka erka deleted the shutdown-https-server branch January 6, 2025 12:12
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.

2 participants