Add signals.RegisterStackTraceHandlerOnSignals for custom handling logic

[bmoylan]

This change is 

[nmiyake]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bmoylan)

a discussion (no related file):
Request that the API be modified so the handler returns an error -- that will prevent clients from having to use the error handler they define themselves in a provided closure (which introduces some awkwardness in requiring parameters to be declared before providing them to the function and mixes some of the concepts)

---

---

signals/signals.go, line 64 at r1 (raw file):

// listener when called.
func RegisterStackTraceWriterOnSignals(out io.Writer, errHandler func(error), sig ...os.Signal) (unregister func()) {
	handler := func(body []byte) {

body doesn't seem semantically correct as the variable name in this scenario.

---

signals/signals.go, line 65 at r1 (raw file):

func RegisterStackTraceWriterOnSignals(out io.Writer, errHandler func(error), sig ...os.Signal) (unregister func()) {
	handler := func(body []byte) {
		if _, err := out.Write(body); err != nil && errHandler != nil {

With proposed API, the function can become:

_, err := out.Write(snapshot)
return err

This makes the API a bit cleaner -- as it stands, is a bit confusing that the error handler passed to the other function is used in the closure here.

---

signals/signals.go, line 73 at r1 (raw file):

// RegisterStackTraceHandlerOnSignals starts a goroutine that listens for the specified signals and calls outHandler with a
// pprof-formatted snapshot of all running goroutines when any of the provided signals are received. If writing to out returns an

The section on "If writing to out returns an error" should be updated/re-worded.

---

signals/signals.go, line 76 at r1 (raw file):

// error, that error is provided to the errHandler function if one is provided. Returns a function that unregisters the
// listener when called.
func RegisterStackTraceHandlerOnSignals(outHandler func([]byte), errHandler func(error), sig ...os.Signal) (unregister func()) {

I think the more direct translation of the previous API is to have outHandler be a func([]byte) error and to pass that returned error to the error handler.

---

signals/signals.go, line 76 at r1 (raw file):

// error, that error is provided to the errHandler function if one is provided. Returns a function that unregisters the
// listener when called.
func RegisterStackTraceHandlerOnSignals(outHandler func([]byte), errHandler func(error), sig ...os.Signal) (unregister func()) {

out probably wasn't the best variable name initially, but in this context I think it makes less sense -- the handler may not actually be writing output. Prefer something more along the lines of stackTraceHandler, pprofHandler, pprofBytesHandler etc. (doesn't have to be any of these specifically)

---

signals/signals.go, line 90 at r1 (raw file):

				err := pprof.Lookup("goroutine").WriteTo(&buf, 2)
				if err != nil && errHandler != nil {
					errHandler(err)

We can keep this fore completeness, but as written this is actually not reachable -- bytes.Buffer never returns a non-nil error in its Write function, and WriteTo only returns the error of the provided writer.

I think it would be acceptable to also do an underscore assignment for the error and to explain that the error cannot be reached based on logic (leaving is also fine, but may be a bit confusing to see the error handling code twice)

---

signals/signals.go, line 93 at r1 (raw file):

				}
				if outHandler != nil {
					outHandler(buf.Bytes())

Returned error should be stored and provided to the errHandler if non-nil

[bmoylan]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nmiyake)

a discussion (no related file):

Previously, nmiyake (Nick Miyake) wrote…

Request that the API be modified so the handler returns an error -- that will prevent clients from having to use the error handler they define themselves in a provided closure (which introduces some awkwardness in requiring parameters to be declared before providing them to the function and mixes some of the concepts)

Agreed and done, thanks for the review!

---

---

signals/signals.go, line 64 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

body doesn't seem semantically correct as the variable name in this scenario.

changed to stackTraceOutput

---

signals/signals.go, line 65 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

With proposed API, the function can become:

_, err := out.Write(snapshot)
return err

This makes the API a bit cleaner -- as it stands, is a bit confusing that the error handler passed to the other function is used in the closure here.

Done.

---

signals/signals.go, line 73 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

The section on "If writing to out returns an error" should be updated/re-worded.

Done.

---

signals/signals.go, line 76 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

I think the more direct translation of the previous API is to have outHandler be a func([]byte) error and to pass that returned error to the error handler.

Done.

---

signals/signals.go, line 76 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

out probably wasn't the best variable name initially, but in this context I think it makes less sense -- the handler may not actually be writing output. Prefer something more along the lines of stackTraceHandler, pprofHandler, pprofBytesHandler etc. (doesn't have to be any of these specifically)

Done.

---

signals/signals.go, line 90 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

We can keep this fore completeness, but as written this is actually not reachable -- bytes.Buffer never returns a non-nil error in its Write function, and WriteTo only returns the error of the provided writer.

I think it would be acceptable to also do an underscore assignment for the error and to explain that the error cannot be reached based on logic (leaving is also fine, but may be a bit confusing to see the error handling code twice)

Done.

---

signals/signals.go, line 93 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Returned error should be stored and provided to the errHandler if non-nil

Done.

[nmiyake]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bmoylan)

a discussion (no related file):

Previously, bmoylan (Brad Moylan) wrote…

Agreed and done, thanks for the review!

Thanks!

In looking at it again, noticed a few more high-level things that I think will improve this:

• Bail out early as a no-op if handler is nil or sig is empty
• Use context to control lifecycle of goroutine instead of a cancel function

See feedback for details.

---

---

signals/signals.go, line 74 at r2 (raw file):

// pprof-formatted snapshot of all running goroutines when any of the provided signals are received. If stackTraceHandler returns
// an error, that error is provided to the errHandler function if one is provided. Returns a function that unregisters the listener when called.
func RegisterStackTraceHandlerOnSignals(stackTraceHandler func(stackTraceOutput []byte) error, errHandler func(error), sig ...os.Signal) (unregister func()) {

Since we're adding this as a new function, let's handle cancellation using a context rather than returning a cancel function -- I've found that it's a much better pattern because it allows the lifecycle of the goroutine to be tied to a larger context, but we can still manually return the cancel function if we want to do so.

So, signature changes to:

func RegisterStackTraceHandlerOnSignals(ctx context.Context, ...) {
}

Then we no longer declare/return a cancel function, and goroutine select adds a case for <-ctx.Done().

To keep back-compat for the previous function:

ctx, cancel := context.WithCancel(context.Background())
RegisterStackTraceHandlerOnSignals(ctx, ...)
return cancel

This should make the usage at the call site easier as well (can pass in the "real" context and don't need to worry about deferring the cancel).

---

signals/signals.go, line 87 at r2 (raw file):

				var buf bytes.Buffer
				_ = pprof.Lookup("goroutine").WriteTo(&buf, 2) // bytes.Buffer's Write never returns an error, so we swallow it
				if stackTraceHandler != nil {

Just noticed this when looking at this again, but if stackTraceHandler is nil, this entire function is basically a no-op (we will write the pprof to bytes.Buffer, but never do anything with it).

I think it makes sense to just check on the no-op cases (len(sig) == 0 being the other one) at the beginning and bail out if it will be a noop:

if stackTraceHandler == nil || len(sig) == 0 {
     return fun(){}
}

Once that's done, we don't need to bother with the nil check.

We should just add to the function documentation that the function is a no-op/no goroutine is started if the handler is nil or no signals are specified.

[bmoylan]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nmiyake and @bmoylan)

---

signals/signals.go, line 74 at r2 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Since we're adding this as a new function, let's handle cancellation using a context rather than returning a cancel function -- I've found that it's a much better pattern because it allows the lifecycle of the goroutine to be tied to a larger context, but we can still manually return the cancel function if we want to do so.

So, signature changes to:

func RegisterStackTraceHandlerOnSignals(ctx context.Context, ...) {
}

Then we no longer declare/return a cancel function, and goroutine select adds a case for <-ctx.Done().

To keep back-compat for the previous function:

ctx, cancel := context.WithCancel(context.Background())
RegisterStackTraceHandlerOnSignals(ctx, ...)
return cancel

This should make the usage at the call site easier as well (can pass in the "real" context and don't need to worry about deferring the cancel).

Done.

---

signals/signals.go, line 87 at r2 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Just noticed this when looking at this again, but if stackTraceHandler is nil, this entire function is basically a no-op (we will write the pprof to bytes.Buffer, but never do anything with it).

I think it makes sense to just check on the no-op cases (len(sig) == 0 being the other one) at the beginning and bail out if it will be a noop:

if stackTraceHandler == nil || len(sig) == 0 {
     return fun(){}
}

Once that's done, we don't need to bother with the nil check.

We should just add to the function documentation that the function is a no-op/no goroutine is started if the handler is nil or no signals are specified.

Done. As discussed in person, we'll match os/signal's API for when no signals are provided.

[nmiyake]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bmoylan)

a discussion (no related file):

Previously, nmiyake (Nick Miyake) wrote…

Thanks!

In looking at it again, noticed a few more high-level things that I think will improve this:

• Bail out early as a no-op if handler is nil or sig is empty
• Use context to control lifecycle of goroutine instead of a cancel function

See feedback for details.

Great, -- thanks!

---