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: initialize shutdown context timeout after interrupt #1057

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

GeorgeMac
Copy link
Member

Just a quick fix for an observation.

The shutdown context and 5 second timeout was getting initialized on Flipt start.
Meaning, the shutdown context was likely (unless Flipt was stopped immediately) to be well and truly cancelled before any interrupt is sent.

This moves the initialization of the 5-second timeout context to be after any interrupt is received.
Ensuring at-least five seconds grace period is given on shutdown (maybe this should be configurable too at some point).

@GeorgeMac GeorgeMac requested a review from markphelps October 5, 2022 10:37
@GeorgeMac GeorgeMac force-pushed the gm/shutdown-context branch from 1d56fdd to 0556496 Compare October 5, 2022 10:53
@markphelps
Copy link
Collaborator

Good find @GeorgeMac !

I added one change, previously we were only logging when the HTTP server shutdown, I added logging for the GRPC server as well:

Before

^C2022-10-05T09:26:33-04:00     INFO    shutting down...

2022-10-05T09:26:33-04:00       INFO    server shutdown gracefully      {"server": "http"}

After

^C2022-10-05T09:22:55-04:00     INFO    shutting down...

2022-10-05T09:22:55-04:00       INFO    grpc server shutdown gracefully {"server": "grpc"}
2022-10-05T09:22:55-04:00       INFO    http server shutdown gracefully {"server": "http"}

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

🕺lgtm!

@codecov-commenter
Copy link

Codecov Report

Merging #1057 (23af915) into main (9296bda) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1057   +/-   ##
=======================================
  Coverage   81.40%   81.40%           
=======================================
  Files          17       17           
  Lines        1828     1828           
=======================================
  Hits         1488     1488           
  Misses        265      265           
  Partials       75       75           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@markphelps markphelps merged commit f94d704 into main Oct 5, 2022
@markphelps markphelps deleted the gm/shutdown-context branch October 5, 2022 13:36
markphelps added a commit that referenced this pull request Oct 5, 2022
…nto list-pagination

* 'list-pagination' of https://github.com/flipt-io/flipt:
  fix: initialize shutdown context timeout after interrupt (#1057)
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