fix: make async webhooks fully async #3111
Conversation
zepatrik
force-pushed
the
fix/async-hook-body-error
branch
from
950a51d to
4f60b2d
Compare
| @@ -332,40 +343,30 @@ func (e *WebHook) execute(ctx context.Context, data *templateContext) error { | |||
| if canInterrupt || parseResponse { | |||
| if err := parseWebhookResponse(resp, data.Identity); err != nil { | |||
| span.SetStatus(codes.Error, err.Error()) | |||
| errChan <- err | |||
There was a problem hiding this comment.
After this there is no return, so another error would be send.
| } | ||
|
|
||
| if parseResponse { | ||
| if err := parseWebhookResponse(resp, data.Identity); err != nil { | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| errChan <- err |
There was a problem hiding this comment.
Same here, after the error nil is send.
| return nil | ||
| } | ||
|
|
||
| return <-errChan |
There was a problem hiding this comment.
This always waits for one message on the channel, I don't understand how it could even run async as this is blocking AND racy with the go routine also reading from that channel 😅
There was a problem hiding this comment.
Ahh it would return in the if above...
Codecov Report
@@ Coverage Diff @@
## master #3111 +/- ##
=======================================
Coverage 77.57% 77.57%
=======================================
Files 315 315
Lines 19962 19963 +1
=======================================
+ Hits 15486 15487 +1
Misses 3285 3285
Partials 1191 1191
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| if !ignoreResponse { | ||
| return makeRequest() | ||
| } | ||
| go func() { | ||
| // we cannot handle the error as we are running async, and it is logged anyway | ||
| _ = makeRequest() | ||
| }() | ||
| return nil |
There was a problem hiding this comment.
| if !ignoreResponse { | |
| return makeRequest() | |
| } | |
| go func() { | |
| // we cannot handle the error as we are running async, and it is logged anyway | |
| _ = makeRequest() | |
| }() | |
| return nil | |
| if ignoreResponse { | |
| go func() { | |
| // we cannot handle the error as we are running async, and it is logged anyway | |
| _ = makeRequest() | |
| }() | |
| return nil | |
| } | |
| return makeRequest() |
Just a nit. I like positive branching. But not sure that it improves this much. It's fine either way.
There was a problem hiding this comment.
Hm I typically go for shorter branches in the if body, as that makes it easier to read.
| req, err := builder.BuildRequest(ctx, data) | ||
| if errors.Is(err, request.ErrCancel) { | ||
| return nil | ||
| } else if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This prints the kratos cli usage help if, the jsonnet is invalid:
I guess it did that before, but can we improve that somehow?
There was a problem hiding this comment.
Yeah, we can do that but separately. I guess it does not hurt though.
Related issue(s)
The current approach does not ignore errors from the request building process, only the actual request. This patch moves the request build into the async part of the webhook as well.
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments
_