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

Gracefully shutdown HTTP, gRPC, and Prom servers #1342

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Aug 31, 2023

This catches sigint and sigterm and calls each server's shutdown method, blocking until shutdown completes.

This is only added for non-Duplex, we'll need to add a shutdown method for Duplex servers.

Fixes #1341

Summary

Release Note

Documentation

This catches sigint and sigterm and calls each server's shutdown method,
blocking until shutdown completes.

This is only added for non-Duplex, we'll need to add a shutdown method
for Duplex servers.

Signed-off-by: Hayden Blauzvern <[email protected]>
@haydentherapper
Copy link
Contributor Author

haydentherapper commented Aug 31, 2023

Tested with go run main.go serve --port 5555 --grpc-port 5554 --ca ephemeralca --ct-log-url="" and confirmed we saw all expected logging statements.

I tried to get this working for Duplex by calling d.Server.GracefulStop(), but the process hung. This is because Serve() doesn't expose the server to stop. Will need to update Duplex, but we don't use this in prod.

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #1342 (cefc964) into main (f94df0a) will decrease coverage by 0.93%.
Report is 9 commits behind head on main.
The diff coverage is 30.43%.

@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
- Coverage   58.49%   57.56%   -0.93%     
==========================================
  Files          50       50              
  Lines        3053     3111      +58     
==========================================
+ Hits         1786     1791       +5     
- Misses       1110     1161      +51     
- Partials      157      159       +2     
Files Changed Coverage Δ
cmd/app/http.go 48.93% <0.00%> (-11.60%) ⬇️
cmd/app/serve.go 25.47% <3.57%> (-1.77%) ⬇️
cmd/app/grpc.go 74.72% <90.90%> (-1.79%) ⬇️

... and 1 file with indirect coverage changes

Signed-off-by: Hayden Blauzvern <[email protected]>
cpanato
cpanato previously approved these changes Sep 1, 2023
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

Cool

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

if ports are already in use (e.g. tcp :2112: bind: address already in use), the server will hang instead of quickly failing and exiting

cmd/app/http.go Show resolved Hide resolved
cmd/app/serve.go Show resolved Hide resolved
@haydentherapper
Copy link
Contributor Author

@bobcallaway Fixed comments, also made the unix domain socket configurable so that you can start up two fulcios on the same machine

@haydentherapper haydentherapper merged commit e91bd33 into sigstore:main Sep 7, 2023
13 checks passed
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.

Add Shutdown behavior
3 participants