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

Only call signal.Notify once during agent startup #4024

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Apr 11, 2018

Calling twice appears to have no adverse effects, however serves to confuse as to what the semantics of such code may be! After some experimentation it seems that the second signal.Notify takes precedence over the first, rather than resulting in two messages being delivered to the signal channel, so there is no change in behaviour resulting from this change.

This seems like it was probably introduced while resolving conflicts during the merge of the fix for #2404.

Calling twice appears to have no adverse effects, however serves to
confuse as to what the semantics of such code may be! This seems like it
was probably introduced while resolving conflicts during the merge of
the fix for hashicorp#2404.
@jen20
Copy link
Contributor Author

jen20 commented Apr 11, 2018

The failure on the Travis build for this PR is unrelated to the change - looks transient to me.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the PR. I will merge this after the 1.0.7 release.

@mkeeler mkeeler added this to the Next milestone Apr 12, 2018
@pearkes pearkes modified the milestones: Upcoming, 1.1.0 Apr 20, 2018
@mkeeler mkeeler merged commit 63250c5 into hashicorp:master Apr 20, 2018
@jen20 jen20 deleted the signal-notify-once branch April 29, 2018 19:00
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