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

feat(*): Support for handling panic error #491

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

linhbkhn95
Copy link
Contributor

@linhbkhn95 linhbkhn95 commented Jun 15, 2022

Why did we need it?

  • Allow users can handle the panic error if they want
    Example:
func main() {

	srv := asynq.NewServer(
		asynq.RedisClientOpt{
			Addr: redisAddr,
		},
		asynq.Config{
			// Specify how many concurrent workers to use
			Concurrency: 10,
			// Optionally specify multiple queues with different priority.
			Queues: map[string]int{
				"critical": 6,
				"default":  3,
				"low":      1,
			},
			ErrorHandler: asynq.ErrorHandlerFunc(HandleErrorFunc),
		},
	)

	// mux maps a type to a handler
	mux := asynq.NewServeMux()
	mux.HandleFunc(tasks.TypeEmailDelivery, tasks.HandleEmailDeliveryTask)
	mux.Handle(tasks.TypeImageResize, tasks.NewImageProcessor())
	// ...register other handlers...

	if err := srv.Run(mux); err != nil {
		log.Fatalf("could not run server: %v", err)
	}
}

// The ErrorHandlerFunc type is an adapter to allow the use of  ordinary functions as a ErrorHandler.
// If f is a function with the appropriate signature, ErrorHandlerFunc(f) is a ErrorHandler that calls f.
// HandleError calls fn(ctx, task, err)
func HandleErrorFunc(ctx context.Context, task *asynq.Task, err error) {
	if asynq.IsPanicError(err) {
		// Handle panic error here.
	}
}

fixed #487

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #491 (f915368) into master (cbb1be3) will increase coverage by 0.03%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   68.34%   68.37%   +0.03%     
==========================================
  Files          27       27              
  Lines        3829     3836       +7     
==========================================
+ Hits         2617     2623       +6     
- Misses        928      929       +1     
  Partials      284      284              
Impacted Files Coverage Δ
server.go 85.47% <ø> (ø)
internal/errors/errors.go 61.44% <66.66%> (+0.19%) ⬆️
processor.go 79.23% <100.00%> (+0.35%) ⬆️

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

@linhbkhn95 linhbkhn95 changed the title feat(*): Allow inject recover func when worker panic Allow inject recover func when worker panic#added Jun 16, 2022
@imcvampire
Copy link

LGTM! Please review and merge it!

@hibiken
Copy link
Owner

hibiken commented Jul 2, 2022

@linhbkhn95 Thank you for opening a PR, and sorry for the delay in response.
I'd like to avoid adding yet another callback in the Config.
I think a better solution is to wrap in a custom error so that users can leverage the ErrorHandler to handle panic.

Please discuss the API in the #487.

@linhbkhn95 linhbkhn95 force-pushed the master branch 4 times, most recently from 7beba7f to b91288c Compare September 21, 2022 02:27
@linhbkhn95 linhbkhn95 changed the title Allow inject recover func when worker panic#added feat(*):support for handling panic error Sep 21, 2022
@linhbkhn95 linhbkhn95 force-pushed the master branch 2 times, most recently from 22f6a37 to 0adf609 Compare September 21, 2022 03:26
@linhbkhn95 linhbkhn95 changed the title feat(*):support for handling panic error feat(*): Support for handling panic error Sep 21, 2022
@linhbkhn95
Copy link
Contributor Author

@hibiken tks for your suggestion. I updated it.

@linhbkhn95
Copy link
Contributor Author

@hibiken Please review it help me

processor_test.go Outdated Show resolved Hide resolved
@kamikazechaser
Copy link
Collaborator

+1 This would be a useful feature.

@ping-localhost
Copy link

This would be very useful to check if a panic happened or just a "boring" error. Hope this will get merged at some point.

Copy link
Collaborator

@kamikazechaser kamikazechaser left a comment

Choose a reason for hiding this comment

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

LGTM, I have been using this feature for a few weeks now.

Maybe github.com/test-go/testify is an unnecessary dependency because asynq already uses github.com/google/go-cmp in other tests so there will be some little inconsistency.

@kamikazechaser
Copy link
Collaborator

@hibiken We can merge this.

@linhbkhn95
Copy link
Contributor Author

@hibiken I don't know whether u have a concern about this PR?
I really wanna use this feature this root repo instead of my repo

@kamikazechaser kamikazechaser merged commit 551b0c7 into hibiken:master Jul 20, 2023
@kamikazechaser
Copy link
Collaborator

Thanks @linhbkhn95! It will be included in the next release tag.

@hibiken
Copy link
Owner

hibiken commented Jul 20, 2023

@kamikazechaser Please discuss API changes with me before merging these PRs.

hibiken added a commit that referenced this pull request Jul 20, 2023
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.

How to implement custom panic recovery
5 participants