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

fix: server shutdown logging and request context #34

Merged
merged 3 commits into from
Jul 15, 2024
Merged

Conversation

Fazt01
Copy link
Contributor

@Fazt01 Fazt01 commented Jul 11, 2024

  • Canceling Server.Run() context no longer cancels requests base context.

    • because I'd argue there is no usecase where you would want to not-so-gracefully shutdown all running requests. More reasonable use case is to let requests finish (for up to default 30 secs, or before SIGKILL), and if they do not finish, there is nothing too much better that force killing the app
  • No Error logging on successful graceful shut down.

    • because it is weird to see error logs when tasks in ECS are swapped as they are supposed to.
  • few comments as I was confused what does the hook actually do (RegisterOnShutdown does not wait for hooks to complete, but then there is explicit waiting for those hooks before Run returns)

related: https://github.com/strvcom/backend-go-template-api/pull/166

@Fazt01 Fazt01 requested a review from a team July 11, 2024 14:10
@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.24%. Comparing base (28d324b) to head (6a623c9).

Files Patch % Lines
http/server.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   63.95%   64.24%   +0.28%     
==========================================
  Files          12       12              
  Lines         652      646       -6     
==========================================
- Hits          417      415       -2     
+ Misses        223      218       -5     
- Partials       12       13       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TomasKocman
TomasKocman previously approved these changes Jul 11, 2024
@Fazt01 Fazt01 merged commit 5750247 into master Jul 15, 2024
3 checks passed
@Fazt01 Fazt01 deleted the fix/server-shutdown branch July 15, 2024 10:52
This pull request was closed.
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