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.

Sign up for GitHub

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

Jump to bottom

graceful shutdown fix #668

Merged
merged 5 commits into from
Jul 22, 2022
Merged

graceful shutdown fix #668

merged 5 commits into from
Jul 22, 2022

Conversation

GrigoriyMikhalkin
Copy link
Contributor

@GrigoriyMikhalkin GrigoriyMikhalkin commented Jun 30, 2022 •
edited
Loading

There's few problems with a graceful shutdown that i tried to fix with this PR.

  1. shutdownChan channel that was used to finish PollNetMapStream didn't work if no PollNetMapStream was running at the time of shutdown. Also, it could stop only single PollNetMapStream routine, as single value was passed to channel instead of closing it.
  2. There is a race condition with function that handles termination signals, as it isn't included in an error group.

kradalby
kradalby reviewed Jul 10, 2022
View reviewed changes
Copy link
Collaborator

@kradalby kradalby left a comment

Choose a reason for hiding this comment

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

Looks reasonable, just some small nits

app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
@GrigoriyMikhalkin
Copy link
Contributor Author

@kradalby I additionally decomposed OIDCCallback method. Otherwise gocyclo complaint about it's complexity.

@kradalby
Copy link
Collaborator

@kradalby I additionally decomposed OIDCCallback method. Otherwise gocyclo complaint about it's complexity.

Can you separate this into a different PR?

@GrigoriyMikhalkin
Copy link
Contributor Author

@kradalby I additionally decomposed OIDCCallback method. Otherwise gocyclo complaint about it's complexity.

Can you separate this into a different PR?

Done

juanfont
juanfont reviewed Jul 21, 2022
View reviewed changes
Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

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

Only one minor thing to be fixed.

app.go Outdated Show resolved Hide resolved
@juanfont juanfont merged commit 581d1f3 into juanfont:main Jul 22, 2022
@juanfont
Copy link
Owner

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kradalby kradalby kradalby left review comments

@juanfont juanfont juanfont approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GrigoriyMikhalkin @kradalby @juanfont