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

os/signal: NotifyContext should communicate the signal via context.WithCancelCause #60733

Closed
fishy opened this issue Jun 12, 2023 · 6 comments
Closed

os/signal: NotifyContext should communicate the signal via context.WithCancelCause #60733

fishy opened this issue Jun 12, 2023 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@fishy
Copy link

fishy commented Jun 12, 2023

What version of Go are you using (go version)?

$ go version
go version go1.20.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fishy/.cache/go-build"
GOENV="/home/fishy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/fishy/.gopath/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/fishy/.gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/nix/store/8v5zwymidmry0wd3lhj6zggskzsvqrfk-go-1.20.4/share/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/nix/store/8v5zwymidmry0wd3lhj6zggskzsvqrfk-go-1.20.4/share/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1590600196=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ cat main.go 
package main

import (
        "context"
        "fmt"
        "os"
        "os/signal"
)

func main() {
        ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt)
        defer cancel()
        select {
        case <-ctx.Done():
                fmt.Println(context.Cause(ctx))
        }
}
$ go run main.go 
^Ccontext canceled

What did you expect to see?

context.Cause(ctx) should return an error containing the info of which signal caused the cancellation.

What did you see instead?
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 12, 2023
@ianlancetaylor
Copy link
Contributor

CC @henvic @Sajmani

@macrombilux

This comment was marked as spam.

Sign in to view
@jdemeyer
Copy link

Should be easy to implement in a backwards-compatible way: https://go.dev/play/p/y5FpFPupOAP

@fishy
Copy link
Author

fishy commented Jun 12, 2023

@jdemeyer I think we would want the signal to be inspectable programmally, so we would need to define an error type instead of using fmt.Errorf, something like this:

type CanceledBySignalError os.Signal

func (cbse CanceledBySignalError) Error() string {
  return fmt.Sprintf("canceled by signal %v", os.Signal(cbse))
}

then users can inspect it via:

select {
case <-ctx.Done():
  cause := context.Cause(ctx)
  var cbse signal.CanceledBySignalError
  if errors.As(cause, &cbse) {
    sig := os.Signal(cbse)
    // Do something with sig
  }
}

but yes once we can agree on how the error type should look like the actual fix is trivial :)

@gopherbot
Copy link

Change https://go.dev/cl/502675 mentions this issue: os/signal: communicate the signal in NotifyContext

@fishy fishy mentioned this issue Jun 12, 2023
Open
@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 13, 2023
@prattmic prattmic added this to the Backlog milestone Jun 13, 2023
@mknyszek
Copy link
Contributor

Am I understanding correctly that this issue has been basically superceded by #60756? Closing as duplicate for now to keep discussion in one place, but feel free to reply and I can reopen. (For some context on process, proposals that get accepted end up tracking work for the proposal in the same issue, so there's no reason to keep this open for that purpose, but I may be missing something.)

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@fishy @mknyszek @prattmic @ianlancetaylor @jdemeyer @gopherbot @macrombilux