proposal: os/signal: new error type to communicate the signal triggered context cancellation

[fishy]

This comes from the need to fix #60733.

In order to provide the information of which signal triggered context cancellation in signal.NotifyContext using context.Cause, we need a new error type (as that's the only type allowed by context cause) to communicate that info back to the user.

The proposal is to define it as a simple struct that can only be used by pointer as an error:

// CanceledBySignalError represents the error can be retrieved via context.Cause
// on the context returned by NotifyContext to inspect which signal caused the
// cancellation.
type CanceledBySignalError struct {
	// The signal caused the context cancellation.
	Signal os.Signal
}

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

So users can inspect the signal via code like:

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

[bcmills]

I think this should be named os.SignalError instead of signal.CanceledBySignalError, so that it may be used for cases beyond just signal.Notify. For example, I could envision a caller transforming an exec.ExitError by checking whether its ProcessState indicates termination by signal and returning a SignalError.

Actually, SignalError would be even more useful if exec.ExitError had a convenience Is method for it:

	err := cmd.Wait()
	if errors.Is(err, os.SignalError{Signal: syscall.SIGABRT}) {
		// err indicates termination due to signal SIGABRT — program probably failed an assertion
		…
	}

[fishy]

@bcmills I agree that os.SignalError is a better name, but I have a few comments/questions regarding that approach:

1. The error message of it becomes ambiguous if it's used beyond signal.NotifyContext, e.g. it's no longer necessarily "context canceled by signal ...", I guess we can have a more generic message there and use fmt.Errorf("context canceled by %w", &os.SignalError{...}) in signal.NotifyContext
2. I feel that using errors.Is on it opens some cans of worms we probably don't want to deal with. os.Signal is an interface so there's no guarantee that it will be comparable, and I think in general we prefer pointer errors when the error is a struct to make sure it's comparable (because in that case we only need to compare the pointer address).

But I think this can work if we use a generic error message, and change the errors.Is to errors.As in your example.

[bcmills]

os.Signal is an interface so there's no guarantee that it will be comparable

In practice it always is:

~/go/src$ git grep 'func .* Signal() {' .
cmd/vendor/golang.org/x/sys/plan9/syscall_plan9.go:func (n Note) Signal() {}
cmd/vendor/golang.org/x/sys/windows/syscall_windows.go:func (s Signal) Signal() {}
sync/cond.go:func (c *Cond) Signal() {
syscall/syscall_js.go:func (s Signal) Signal() {}
syscall/syscall_plan9.go:func (n Note) Signal() {}
syscall/syscall_unix.go:func (s Signal) Signal() {}
syscall/syscall_wasip1.go:func (s Signal) Signal() {}
syscall/syscall_windows.go:func (s Signal) Signal() {}

syscall.Signal and windows.Signal are each always an integer type when defined. syscall.Note and plan9.Note are string types.

sync.Cond isn't intended to implement os.Signal at all, and doesn't today because it lacks a String method.

So I'm not worried about somebody stuffing an incomparable os.Signal into a SignalError.

[fishy]

I believe that when you use errors.Is(err, os.SignalError{Signal: syscall.SIGABRT}) and err indeed unwraps to os.SignalError type, you need both the unwrapped error and the target to be comparable to not panic. Here you can control the target to be comparable (as you know syscall.SIGABRT is comparable), you have no control what the err is and it's always comparable, so this is still a hanging knife that could panic the code unexpectly.

^ wrong, see https://go.dev/play/p/qpyJXU05CFl

[bcmills]

errors.Is already handles comparability:
https://cs.opensource.google/go/go/+/refs/tags/go1.20.5:src/errors/wrap.go;l=48;drc=4a0a2b33dfa3c99250efa222439f2c27d6780e4a

[fishy]

hmm you are right. tested on playground: https://go.dev/play/p/qpyJXU05CFl

but another issue with making the nonpointer type to implement error is that people often don't know which type to use with errors.As, for example they can make this mistake:

var se *os.SignalError // they thought it would be the pointer type
if errors.As(err, &se) {
  // they'd never get here
}

the solution to that is to also implement os.SignalError.As to support both pointer and non-pointer types, but is that something we consider worth doing here?

[bcmills]

To be clear, I think it would be ok for the type implementing error to be *SignalError.

(exec.ExitError.Is could follow the pointer explicitly.)

[fishy]

But if we use pointer type, errors.Is will no longer work as you expected here, because it will only return true if the pointers are the same, not the contents: https://go.dev/play/p/VNQ-iy4vA_2

[bcmills]

The error type can provide an explicit Is method. 🙃
https://go.dev/play/p/_DxO_e8nZAu

[fishy]

yes but then you need to deal with the comparable check of Signal interface in that Is function (if you just use == blindly you would get unexpected panics)

which kind of goes back to the same question as As in #60756 (comment): is this something we consider worth doing here?

[bcmills]

You don't need to check whether the Signal implementation is comparable. Every real implementation is.