Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: [#521] User can custom recover when a request panic #103
feat: [#521] User can custom recover when a request panic #103
Changes from 40 commits
e06c940
09245fe
5168d1e
32ee690
29d36ee
88daede
e82b340
f3efb50
61b20aa
a771bce
3b77bd0
cc7a261
21125ef
f3c666a
adf6a8c
0a3998c
d001e4d
28af043
b0a62cd
59ae234
5745c8f
fae6fbe
98f64a5
076581a
d5d8a93
998e3c4
9a3074c
a019972
3f2edef
f7a0ac6
36978b1
92c38d4
f66dadd
3915909
faf902e
cf0b756
0023e51
3c5f3bc
fb1faf6
8f27f62
49f6b21
0e34013
99e06c8
c6d79c8
e0727a7
8a0c44e
173e471
e4a1360
f30e2f4
6217114
35e0182
ace488a
f7f209e
440ba87
ca0c57f
4e8ddfb
dfa4bd0
44b41f6
4346508
1682fe7
60320c1
28dcae8
4aceba8
601dbd0
a79cae7
a581045
5a0101e
1c07943
f020930
a0b8834
47c53e7
343d493
41d57a1
057f865
4d82e79
e6fb55f
5df7701
aa56ca1
66e139e
46fc772
48144d5
72a5064
e7eef7b
ee526b3
4d7e660
5371bd7
cdf7d2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thanks for the optimization, however, I removed the nilaway check, and LogFacade shouldn't be nil, so we can keep the original code.
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.
Ditto
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.
Does not use the global
recoverFunc
?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.
I think that a panic that occurs in Timeout or any other middleware is passed down the call stack back to the global Recover handler, which is higher in the chain.
Am I right or wrong?
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.
we can write tests and check this
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.
No, the global recovery can't catch the panic that was thrown in goroutine.
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.
ok, can you tell me how to add a global Recover to a goroutine?
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.
You can save the custom recovery to a variable in
func (r *Route) Recover(callback func(ctx context.Context, err any))
, then you can use the custom recovery here andfunc (r *Route) GlobalMiddleware(middlewares ...httpcontract.Middleware)
.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.
I think that for the proposed option you need to change the Timeout structure:
or
and add the recoverCallback field to Route
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.
It's unnecessary, a global variable is enough.
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.
Please add a test case to test the custom recover as well.
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.
We can keep this line, it will be used when the user doesn't set the global recover and the timeout middleware.
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.
Sorry, mistake, should be:
Please modify https://github.com/goravel/framework/pull/723/files#diff-37b488415e77368d5640d20902b15e5603864dc8c5c1e715bc36e2a8510b81f8R19 synchronously.
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.
There is a
CustomRecovery
method in Gin, we can use it directly.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.
will we end up using gin.CustomRecovery?
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.
If it can be used directly, yes, we can.
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.
Is
CustomRecovery
suitable, please?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.
Sort it:
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.
Remove it, it's unnecessary.
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.
Thanks for the optimization, however, I removed the nilaway check, and ViewFacade shouldn't be nil, so we can keep the original code.
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.
Ditto