-
Notifications
You must be signed in to change notification settings - Fork 3
Extends the Function Stage to Handle Panics #29
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
Conversation
84239e3
to
42898bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a panic‐handling extension so that client code run by the Function
stage can surface panics through a configurable handler.
- Introduces a
StagePanicHandlerAware
interface,StagePanicHandler
type, andFromPanicValue
helper. - Adds
WithStagePanicHandler
option toPipeline
and wires the handler into stages that support it. - Implements panic recovery in the
Function
stage and adds tests for the new handler behavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pipe/pipeline_test.go | Added t.Run subtests for a successful function and a panic‐with‐handler case |
pipe/pipeline.go | Added panicHandler field, WithStagePanicHandler option, and wiring logic |
pipe/panic.go | Defined StagePanicHandlerAware , StagePanicHandler , and FromPanicValue |
pipe/function.go | Implemented SetPanicHandler on goStage and panic recovery in Start |
Comments suppressed due to low confidence (1)
pipe/function.go:56
- The code path where no panic handler is provided (nil handler) and a panic should bubble up is not covered by tests; consider adding a test to verify that a panic propagates when no handler is set.
if s.panicHandler == nil {
Stages can be extended to allow panics to be caught and reported. The panic will still bubble up once the panic handler has resolved.
Clients set the panic handler at the pipeline level. The pipeline is then responsible for passing the handler to stages that support the PanicHandlerAware interface.
42898bc
to
11ad4d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Introduces a mechanism to recover and surface panics in the Function
stage by allowing clients to register a panic handler at the pipeline level.
- Add a
StagePanicHandlerAware
interface andStagePanicHandler
type for stages to receive handlers. - Extend
Pipeline
with aWithStagePanicHandler
option and propagate the handler to supporting stages. - Wrap
Function
stage execution in arecover
block to invoke the handler (or re-panic if none is set).
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
pipe/pipeline_test.go | Add subtests for "successful function" and "panic with handler" scenarios |
pipe/pipeline.go | Store a panic handler on the pipeline and assign it to stages before start |
pipe/panic.go | Define StagePanicHandlerAware , StagePanicHandler , and FromPanicValue |
pipe/function.go | Add recover in goStage to invoke the panic handler or re-panic |
Comments suppressed due to low confidence (1)
pipe/pipeline_test.go:465
- [nitpick] Consider adding a test case for a panic without a registered handler to verify that the pipeline will re-panic as expected when no handler is provided.
panicked := make(chan bool, 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a super reasonable way to implement this to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the Function
stage to recover from panics in its goroutine and invoke a client-provided panic handler.
- Introduces a
StagePanicHandlerAware
interface,StagePanicHandler
type, andFromPanicValue
helper. - Adds a
panicHandler
field andWithStagePanicHandler
option toPipeline
, and wires the handler into stages. - Updates
goStage
to recover panics, close streams, set the stage error, and call the handler when provided.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pipe/pipeline_test.go | Adds tests for a successful function stage and a panic with handler |
pipe/pipeline.go | Adds panicHandler field, WithStagePanicHandler option, and stage wiring |
pipe/panic.go | Defines StagePanicHandlerAware , StagePanicHandler , and FromPanicValue |
pipe/function.go | Implements SetPanicHandler and wraps StageFunc execution in recover |
Comments suppressed due to low confidence (1)
pipe/pipeline_test.go:462
- Add a test case for the default panic behavior when no handler is provided to ensure the pipeline either panics or returns the expected error.
})
351c7ce
to
9c58e5a
Compare
@mhagger I would also appreciate your thoughts on this change as well before I consider merging 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't have to be done in this PR, but it might be cool to add a function that can wrap an arbitrary stage with a panic handler, similar to what FilterError()
does for errors. That might look like
func HandlePanics(s Stage, handler StagePanicHandler) Stage {…}
This would give the user an easy way to wrap arbitrary stages, each with an arbitrary handler, rather than wrapping only Function
stages and always using the same handler. Probably not often useful, but it could reduce coupling and make things a little bit more flexible at the same time.
That random thought aside, I left some other line comments for you to consider.
- StagePanicHandler siganture is updated to take in `any` to avoid discarding information. The return - Panic is wrapped after calling StagePanicHandler and error is returned for the stage so that we can verify the pipeline's error contains the stage panic. - Skip recovery if panic-handler is not set to ensure previous behavior remains unchanged. - Cleanup resources on any type of exit from stage.Start to ensure. - Move recovery logic to a separate method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the changes!
Overview
Certain stages, such as
pipe.Function
take in a client function to run. In the event of a panic, the client code cannot handle any panics that happen in the client function because it is run in a separate goroutine.This PR introduces a panic handler mechanism with the intent of surfacing the panic to the client so that they can react to it accordingly, if they wish. Currently only the
pipe.Function
will support this new behavior as it was the most likely contender for causing panics as it allows any client go code to be run.Implementation
StagePanicHandlerAware
interface that is used to mark stages as capable of receiving a panic handler.pipe.Function
with panic recover code. If a panic handler was provided at the pipeline level, it will be called. Otherwise the panic will bubble up as before.pipe.Pipeline
with a newWithStagePanicHandler
option to allow clients to set a panic handler. The handler is passed into stages that support theStagePanicHandlerAware
interface.Example