Rectify error handling #44
Conversation
command.go
Outdated
| return nil | ||
| } | ||
|
|
||
| return ctx.Err() |
There was a problem hiding this comment.
And what happens to err ? Looks like simply getting lost ?
There was a problem hiding this comment.
how about swapping the conditiona and returning err if not nil and return nil outside the condition block ?
There was a problem hiding this comment.
The err returned by exec.Wait is discarded in favor of ctx.Err(). Because exec.Wait() will returns a string 'signal: killed', which isn't good to depend on and is less meaningful. I don't have a good idea to encode the two errors (exec.Wait's and ctx.Err's) into one cleanly either.
There was a problem hiding this comment.
Do you mean logging the error of Wait and returning the error of ctx.Err() or something else?
| // 'signal: killed' when the error is returned by exec.Wait | ||
| // It depends on the point of the time (before or after exec.Start returns) at which the timeout is triggered. | ||
| if err.Error() != "context deadline exceeded" && err.Error() != "signal: killed" { | ||
| if err != context.DeadlineExceeded { |
There was a problem hiding this comment.
Why do you want to remove documentation in those comments ?
There was a problem hiding this comment.
Since that is encapsulated into RunInDirTimeoutPipeline and the test won't be exposed to the nuances anymore (which is what I hope for).
typeless
force-pushed
the
rectify-error-handling
branch
from
c62f66a
to
c981984
Compare
typeless
force-pushed
the
rectify-error-handling
branch
from
c981984
to
1a38a29
Compare
| @@ -70,7 +70,11 @@ func (c *Command) RunInDirTimeoutPipeline(timeout time.Duration, dir string, std | |||
| return err | |||
| } | |||
|
|
|||
| return cmd.Wait() | |||
| if err := cmd.Wait(); err != nil { | |||
| log("exec.Wait: %v", err) | |||
There was a problem hiding this comment.
should it be log.Warn instead, and include more info about the command being run and the name of the function it's coming from ?
There was a problem hiding this comment.
That would be a bit of more involved since there is an existing function called log within the package. I can give a nickname to the logger at the import statement but that looks weird and inconsistent with the rest of the package. The plan B is to rename the internal log function with another name, or to migrate all the uses of the log function with the logger's equivalent. But that would be beyond the scope of this PR.
There was a problem hiding this comment.
Oops, sorry. I just realized this was the git module and not gitea itself.
|
LGTM |
|
LGTM |
For #42 and #41
Sanitizing the error handling a little bit, now it looks nicer.