go-sdk icon indicating copy to clipboard operation
go-sdk copied to clipboard

Error Handling Bugs

Open armsnyder opened this issue 3 years ago • 1 comments

Hi, while trying out the SDK I noticed some instances of incomplete error handling. This isn't a very comprehensive list; it's just what I saw while looking at the actor code primarily.

Bugs

  • [ ] NewClient() can return a nil error on a repeat invocation, because the onceErr is in the function scope. (#280)
  • [ ] ImplActorClientStub() and its child implActor() do not notify the caller if an error was encountered. Maybe add a new method with an error return type, or change it to panic() on errors for now.
  • [ ] RegisterActorFactory() ignores the NewDefaultActorManager() error and returns nil to the caller without even logging.
  • [ ] Invoking an actor method on a client stub can hit an error condition and not notify the caller.
    • Example of an error that is logged without being returned to the caller: https://github.com/dapr/go-sdk/blob/7c38f5a607d58ee0af93a2f26a061d5c1834203a/client/actor.go#L392-L393
    • Example of an error which is an ineffectual assignment. The err variable is unused after this point: https://github.com/dapr/go-sdk/blob/7c38f5a607d58ee0af93a2f26a061d5c1834203a/client/actor.go#L404
  • [ ] The http service implementation does not log the error if one was encountered (for example, an issue with a custom serializer), making debugging difficult.

Other areas for improvement

  1. I noticed you are using the github.com/pkg/errors package for creating and wrapping error types. Since Go 1.13, error wrapping is built into the standard errors and fmt libraries, so you could remove this dependency.
  2. Some of these bugs can be caught and enforced with a linter. I recommend golangci-lint, which has errcheck and ineffassign enabled by default.

armsnyder avatar Aug 22 '22 16:08 armsnyder

I think it could be worth segregating them as separate issues so that they can be PR´d individually.

spolab avatar Jan 03 '23 20:01 spolab