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

Add panic recovers #1251

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Conversation

AlgoStephenAkiki
Copy link
Contributor

Resolves #263

Adds recovery functions in main loop of conduit as well as in a go-routine in the start function of the pipeline

Summary

Explain the goal of this change and what problem it is solving. Format this cleanly so that it may be used for a commit message, as your changes will be squash-merged.

Test Plan

How did you test these changes? Please provide the exact scenarios you tested in as much detail as possible including commands, output and rationale.

Resolves #263

Adds recovery functions in main loop of conduit as well as in a
go-routine in the start function of the pipeline
@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #1251 (a831389) into conduit (c809d35) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           conduit    #1251      +/-   ##
===========================================
- Coverage    62.06%   62.05%   -0.01%     
===========================================
  Files           70       71       +1     
  Lines         9434     9440       +6     
===========================================
+ Hits          5855     5858       +3     
- Misses        3073     3075       +2     
- Partials       506      507       +1     
Impacted Files Coverage Δ
conduit/common.go 25.00% <25.00%> (ø)
conduit/pipeline.go 52.50% <100.00%> (+0.34%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 53 to 57
defer func() {
if r := recover(); r != nil {
logger.Fatalf("conduit framework experienced a panic: %v", r)
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worth making a helper function? I'm thinking that could be something we document / reuse in plugins.

	func HandlePanicAndFail(logger *log.Logger) {
		if r := recover(); r != nil {
			logger.Panicf("conduit framework experienced a panic: %v", r)
		}
	}

Also, it looks like logrus has a "Panic" level. The difference compared to Fatal is that it calls panic after logging instead of Exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-worked, let me know what you think.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AlgoStephenAkiki AlgoStephenAkiki merged commit a6b7473 into conduit Oct 3, 2022
@AlgoStephenAkiki AlgoStephenAkiki deleted the 263-catch-panics-and-write-to-log branch October 3, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants