Make sure to call after hook on error #1479
Conversation
| for ; hookIndex < len(hs.hooks); hookIndex++ { | ||
| ctx, retErr = hs.hooks[hookIndex].BeforeProcess(ctx, cmd) | ||
| if retErr != nil { | ||
| cmd.SetErr(retErr) |
There was a problem hiding this comment.
Right, but why that is a problem? I call AfterProcess for all functions except the last one that returned the error.
| cmd.SetErr(err) | ||
| return err | ||
| if retErr == nil { | ||
| retErr = fn(ctx, cmd) |
There was a problem hiding this comment.
if retErr != nil -> cmd.SetErr(retErr)
There was a problem hiding this comment.
Yes, cmd.SetErr is not needed here - fn sets the cmd.err
redis.go
Outdated
| var hookIndex int | ||
| var retErr error | ||
|
|
||
| for ; hookIndex < len(hs.hooks); hookIndex++ { |
There was a problem hiding this comment.
maybe
for ; retErr == nil && hookIndex < len(hs.hooks); hookIndex++
looks nice,
and idx-- is not required at afterprocess starts.
There was a problem hiding this comment.
I believe with retErr == nil I need idx -= 2 if I don't want to include the hook that returned the error. Am I missing something?
There was a problem hiding this comment.
example:
beforeProcess is stop at i'th idx (i>=0), afterProcess should starts at hooks[i], not hooks[i-1]
There was a problem hiding this comment.
code:
we assume beforehook would stop at 5'th hook, all hooks=[0, 1, 2, 4, 5, 6, 7]
// before hook [0, 1, 2, 3, 4, 5]
for ; hookIndex < len(hs.hooks); hookIndex++ {
if err != nil { // err at 5'th hook
break
}
}
// hookIndex = 5
// after hook, only executes hooks [4, 3, 2, 1], 5'th is lost.
for hookIndex--; hookIndex >= 0; hookIndex-- {
// it will start from hookIndex=4, 5'th hook is lost.
}There was a problem hiding this comment.
Yes, I intentionally skipped the AfterProcess hook that returned the error from BeforeProcess. Since I don't have strong preference I've changed the code to include it.
Please take another look.
There was a problem hiding this comment.
i think afterhook focus on handling result and error.
There was a problem hiding this comment.
example:
there is a hook registered in hooks at i'th index,
beforehook is focus on authentification,
afterhook records log and emit metrics, the error msg is used in afterhook.
vmihailenco
force-pushed
the
fix/hook-call-after
branch
from
1bcf604 to
0482677
Compare
redis.go
Outdated
| ctx, err = h.BeforeProcess(ctx, cmd) | ||
| if err != nil { | ||
| return nil, err | ||
| for ; hookIndex >= 0; hookIndex-- { |
There was a problem hiding this comment.
another issues:
if beforehooks run all success, hookIndex is equals to len(hooks),
afterhooks will be run out of range.
that's why
using
for ; err == nil && hookIndex < len(hs.hooks); hookIndex++
instead of
for ; hookIndex < len(hs.hooks); hookIndex++ is better.
you don't need to check hookIndex is out of range. :)
There was a problem hiding this comment.
You are right - fixed the code
vmihailenco
force-pushed
the
fix/hook-call-after
branch
2 times, most recently
from
8fe4c4b to
c30c531
Compare
redis.go
Outdated
| ctx, retErr = hs.hooks[hookIndex].BeforeProcessPipeline(ctx, cmds) | ||
| if retErr != nil { | ||
| setCmdsErr(cmds, retErr) | ||
| break |
There was a problem hiding this comment.
Thanks, removed
vmihailenco
force-pushed
the
fix/hook-call-after
branch
from
c30c531 to
69287d7
Compare
|
LGTM 👍 |
|
@kaka19ace thanks for the review! |
Fixes #1478
@kaka19ace please review.