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

x/simulation: unbuffered os.Signal channel passed into signal.Notify isn't correct #7950

Closed
4 tasks done
odeke-em opened this issue Nov 16, 2020 · 0 comments · Fixed by #7952
Closed
4 tasks done

x/simulation: unbuffered os.Signal channel passed into signal.Notify isn't correct #7950

odeke-em opened this issue Nov 16, 2020 · 0 comments · Fixed by #7952
Assignees
Labels

Comments

@odeke-em
Copy link
Collaborator

odeke-em commented Nov 16, 2020

Noticed while auditing and testing code, as of tip at be10bcb, the code in x/simulation looks like this

// Setup code to catch SIGTERM's
c := make(chan os.Signal)
signal.Notify(c, os.Interrupt, syscall.SIGTERM, syscall.SIGINT)

but that channel is unbuffered, which means that if signals are sent before the channel is read from, they'll be dropped.
The docs for os/signal.Notify at https://golang.org/pkg/os/signal/#Notify explicitly call this out

Screen Shot 2020-11-16 at 2 55 16 PM

In Go1.17, I plan on producing a static analyzer that'll automatically flag these when go test runs, but my company will release it even earlier as per golang/go#9399.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@odeke-em odeke-em added the T:Bug label Nov 16, 2020
@odeke-em odeke-em self-assigned this Nov 16, 2020
odeke-em added a commit that referenced this issue Nov 17, 2020
Invoking:

    c := make(chan os.Signal)
    signal.Notify(c, signals...)

is explicitly called out as a bug in the os/signal docs
and should instead make that channel buffered lest
a signal could be lost.

Fixes #7950
@mergify mergify bot closed this as completed in #7952 Nov 17, 2020
mergify bot pushed a commit that referenced this issue Nov 17, 2020
…7952)

Invoking:

    c := make(chan os.Signal)
    signal.Notify(c, signals...)

is explicitly called out as a bug in the os/signal docs
and should instead make that channel buffered lest
a signal could be lost.

Fixes #7950

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant