[BUG] error handling needs to be done correctly about grofer HOT 12 CLOSED

can you implement the same for the parts as well?

Ok, I'll do the same and open PR. 😃

from grofer.

Sorry for posting in a short time. There's another solution. How do you think, @Gituser143 @MadhavJivrajani ?

• Defines a custom error in somewhere in general (for example, general/errors.go), instead of errCanceledByUser in cmd/root.go.
• RenderCPUinfo() returns the custom error when q or Ctrl-C pressed, and returns ctx.Err() when receiving from ctx.Done().

from grofer.

The solution looks good! It'll ensure that returns are non-empty too! We could use this custom error in other functions used with grofer -c and grofer proc too.

If you'd implement this, it would be great! 😄

from grofer.

I concur with @Gituser143 this looks great, thanks!

from grofer.

Hi.
I read this proposal, and I've made some rough fixes.
master...egawata:wip_fix_error_handling

• Adds a channel to receive error from info.GetCPULoad.
• Uses a context instead of channels, to communicate between goroutines. This would make a communication between goroutines a bit simpler.
• Pressing q works well to exit the program.

If this change helps fix this issue, I'll make a PR.

from grofer.

That's a better approach @egawata , thanks!
That does simplify error handling in this case. This got me searching for better ways to handle errors in a concurrent setting and I'd like your opinion on using errgroup which is basically meant for synchronizing, handling and passing errors occurred in one or more related goroutines. The idea remains the same as you suggested, using contexts but this might simplify things further in our case.

Let me know what you think!

from grofer.

@MadhavJivrajani
Thank you for your kind advice! I've tried rewriting it with errgroup.
master...egawata:wip_fix_error_handling_by_errgroup

I also think that is very simple and right solution to our case. 😄

from grofer.

It looks good for the most part, error handling seems to work great, I had a few queries which I was hoping you could address:

• Why is errCanceledByUser a new type of error?
• Since you've use errgroup group, instead of returning errors in each eg.Go(...) would it be better to call eg.Wait() since that will collectively wait for all of the associated goroutines to complete as well as collect errors from them and return it back to the caller?

My reference

from grofer.

Thank you for your advice 😃

Why is errCanceledByUser a new type of error?

All goroutines are canceled when one of eg.Go() in errgroup returns non-nil error. To accomplish this, each eg.Go() section should return error, even if there is actually no error.
Now overallGraph.RenderCPUinfo in the second eg.Go() returns no error (maybe when q pressed), so I make it return dummy error (errCanceledByUser) Of course, it shouldn't be handled as an error.

would it be better to call eg.Wait() since that will collectively wait for all of the associated goroutines to complete as well as collect errors from them and return it back to the caller?

In my code, it seems eg.Wait() would be called, wait until all eg.Go() to complete, and return the first error (if any) properly.
If you think it is better to return the error to its caller (meaning cobra), only adding return err will do that, although it makes error messages too verbose.

Please give me your opinion if I've misunderstood something. Thanks!

from grofer.

Oh alright, that makes sense, please go ahead and open the PR and we can move forward with this!

Currently, you've implemented error handling only for grofer -c, can you implement the same for the parts as well? Starting with the else clause in root.go, error handling would need to be implemented for all aspects that have a TUI component which includes grofer proc [-p PID][-r refresh_rate].

For example, in serveStats.go, errors are not being handled correctly.

Thanks!

from grofer.

@egawata when you receive from the channel of ctx.Done wouldn't it make sense to return ctx.Err() instead of a simple return? What I see from your changes to RenderCPUinfo() function, you're waiting on the return values of this function back in root.

If there's something I'm missing please let me know. Thanks!

from grofer.

@Gituser143
Thank you for your opinion! 😃
I think it is a good style of func to return an error which indicates the reason, or nil if everything has gone well. I agree returning ctx.Err() is better than a simple error.
The main reason why I don't do that is that the error will always be discarded. At the time the routine in RenderCPUinfo() receives from ctx.Done channel, other goroutine in the same errgroup has already returned a non-nil error. eg.Wait() returns only the first non-nil error from other goroutine, not RenderCPUinfo().

Again, I think adding a return value of type error to RenderCPUinfo() is good, but we would be better wait to do it until we really need it. How do you think?

from grofer.