The Wayback Machine - https://web.archive.org/web/20210911024123/https://github.com/arduino/arduino-cli/pull/1251
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better gRPC error handling #1251

Merged
merged 19 commits into from Aug 30, 2021
Merged

Better gRPC error handling #1251

merged 19 commits into from Aug 30, 2021

Conversation

@cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 30, 2021

This is a proposal to uniform gRPC error handling.

For now only the upload command has been ported, as a proof-of-concept. It will be extended to all the arduino-cli code base if approved.

This PR, once fully implemented, should:

  • change all gRPC API implementations to return a status.Status instead of an error (the Status message is how the gRPC transports errors on RPC calls).
  • application-specific errors must be mapped to gRPC codes
  • application-specific errors that may be useful to "identify" client-side are reported also in the Status.details field (for example the UploadRequiresProgrammerError)
commands/upload/upload.go Outdated Show resolved Hide resolved
Copy link
Contributor

@silvanocerza silvanocerza left a comment

I like this, it's a great improvement that's very much needed. I just have some doubt about the returning an rpc.UploadResponse even when the status.Status might contain errors.

@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from 273cffa to 7c630e7 Jun 23, 2021
@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch 2 times, most recently from 64b9066 to adfa4a2 Jul 13, 2021
@cmaglie cmaglie marked this pull request as ready for review Jul 13, 2021
Copy link
Contributor

@per1234 per1234 left a comment

Is this considered a breaking change to the API?

If so, does the standard process for documenting it need to be done still?
https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-request-checklist

@cmaglie
Copy link
Member Author

@cmaglie cmaglie commented Jul 15, 2021

🤦🏼 right, we should prepare a migration guide.

I'll write down some notes later today...

@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from af977d4 to c04a3d7 Aug 10, 2021
cli/core/upgrade.go Outdated Show resolved Hide resolved
@@ -75,13 +75,13 @@ func ParseLibraryReferenceArgs(args []string) ([]*LibraryReferenceArg, error) {
// ParseLibraryReferenceArgAndAdjustCase parse a command line argument that reference a
// library and possibly adjust the case of the name to match a library in the index
func ParseLibraryReferenceArgAndAdjustCase(instance *rpc.Instance, arg string) (*LibraryReferenceArg, error) {
libRef, err := ParseLibraryReferenceArg(arg)
libRef, _ := ParseLibraryReferenceArg(arg)

This comment has been minimized.

@silvanocerza

silvanocerza Aug 11, 2021
Contributor

Shouldn't we fix this by handling this err instead of ignoring it?

This comment has been minimized.

@per1234

per1234 Aug 11, 2021
Contributor

Probably already clear, but I did this because this assignment of err was ignored and the error type from the declaration was incompatible with the change of lib.LibrarySearch()'s return type from error to *status.Status.

If it's to be ignored, I think it's best to make that explicit by using _. But if an error return from ParseLibraryReferenceArg() is significant in this context, then of course it's better to handle it.

This comment has been minimized.

@cmaglie

cmaglie Aug 11, 2021
Author Member

Yep, the err was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.

client_example/main.go Outdated Show resolved Hide resolved
@silvanocerza
Copy link
Contributor

@silvanocerza silvanocerza commented Aug 11, 2021

The translation files must be updated, the rest is good. I'll approve as soon that's fixed. 👍

@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from c04a3d7 to acab430 Aug 11, 2021
@cmaglie
Copy link
Member Author

@cmaglie cmaglie commented Aug 24, 2021

After trying the current solution for a bit I found two annoying problems:

  1. the error messages are not consistent, just to make an example the snippet:

    	packageManager := i.PackageManager
    	if packageManager == nil {
    		return nil, status.New(codes.InvalidArgument, tr("Invalid instance"))
    	}

    is repeated a lot in the codebase, but there are cases where the message is different:

    		return nil, errors.Errorf(tr("incorrect instance")
    		return nil, errors.Errorf(tr("unable to find an instance with ID: %d"), instanceID)
  2. developers that imports the commands package directly will have the following code broken:

    detailsResp, err := board.Details(context.Background(), detailsReq)
    if err != nil {
        fmt.Println("Error getting board details:", err)
        return
    }

    even if it seems perfectly reasonable it prints an address instead of an error message, because *status.Status did not
    implement a String() method, this is not idiomatic.

PROPOSAL

To address the two problems above, while rebasing this PR on master, I'd like to try a different approach:

  1. to avoid repetition of error strings we define a bunch of common errors objects in commands/errors.go, something like:

    type InvalidInstanceError struct{}
    
    func (e *InvalidInstanceError) Error() string {
           return tr("Invalid instance")
    }
  2. to make life easier for developers importing the commands package I'll try to:

    • set back the return type to error instead of *status.Status.
    • define a CommandError interface:
      type CommandError interface {
             ToRPCStatus() *status.Status
      }
    • implement the CommandError interface on all errors returned from the commands public API, in this case the InvalidInstanceError show above will become:
      type InvalidInstanceError struct{}
      
      func (e *InvalidInstanceError) Error() string {
             return tr("Invalid instance")
      }
      func (e *InvalidInstanceError) ToRPCStatus() *status.Status {
             return status.New(codes.InvalidArgument, e.Error())
      }
    • the wrappers in deamon.go will use an helper function to convert the error in the correct status.Status:
      func convertError(err error) error {
             if rpcErr, ok := err.(CommandError); ok {
                     return rpcErr.ToRPCStatus().Err()
              }
             return err
      }

@silvanocerza
Copy link
Contributor

@silvanocerza silvanocerza commented Aug 24, 2021

Yes, I 100% agree with this new design. I think it also make it easier to handle errors internally and recover them gracefully.
Let's go! 🐎

@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch 7 times, most recently from 4a6bd7c to c146dae Aug 25, 2021
@cmaglie cmaglie changed the title Proposal: better gRPC error handling Better gRPC error handling Aug 27, 2021
@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from c146dae to b9ecfa6 Aug 27, 2021
arduino/cores/fqbn.go Outdated Show resolved Hide resolved
commands/core/uninstall.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/errors.go Outdated Show resolved Hide resolved
commands/upload/upload.go Outdated Show resolved Hide resolved
per1234 and others added 13 commits Jul 9, 2021
Previously, a custom error was returned when attempting to upgrade a platform that was already at the latest available
version. There is dedicated code for handling this specific error.

Now that the function has been changed to return a status.Status instead of error, the previous check for the return
being this error is no longer possible. The capability is restored by replacing the error with a protocol buffer message.
The status details of the function are used to identify the specific cause of a non-nil status. This is done via a type
assertion. Previously, this type assertion was configured such that a details of any type other than the expected would
result in a panic. At the moment, that will not occur because we only add details of one type. However, the whole point
of the type assertion is to support details of multiple types, and if other types are added a panic will not be the
appropriate behavior.

A better approach is to check the result of the type assertion, handling the non-nil status as a generic error if its
details are of a different type.
@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from 5a405ad to 5587ded Aug 28, 2021
@per1234 per1234 dismissed their stale review Aug 28, 2021

Requested changes have been made. Thanks!

@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch 2 times, most recently from 777b59c to 63dd036 Aug 30, 2021
cli/lib/check_deps.go Outdated Show resolved Hide resolved
commands/board/list.go Outdated Show resolved Hide resolved
commands/upload/upload.go Outdated Show resolved Hide resolved
Copy link
Contributor

@silvanocerza silvanocerza left a comment

All in all it's a great improvement but there are some small things to fix.

cmaglie and others added 2 commits Aug 30, 2021
@cmaglie cmaglie force-pushed the cmaglie:proper-errors branch from 63dd036 to 6bf77b9 Aug 30, 2021
@cmaglie cmaglie merged commit 75b9760 into arduino:master Aug 30, 2021
60 checks passed
60 checks passed
@github-actions
run-determination
Details
@github-actions
run-determination
Details
@github-actions
lint
Details
@github-actions
run-determination
Details
@github-actions
lint
Details
@github-actions
run-determination
Details
@github-actions
run-determination
Details
@github-actions
run-determination
Details
@github-actions
check-errors (./)
Details
@github-actions
check
Details
@github-actions
links
Details
@github-actions
build
Details
@github-actions
formatting
Details
@github-actions
build
Details
@github-actions
test (ubuntu-latest)
Details
@github-actions
test (ubuntu-latest)
Details
@github-actions
check-errors (arduino/discovery/discovery_client)
Details
@github-actions
test (windows-latest)
Details
@github-actions
test (windows-latest)
Details
@github-actions
check-errors (client_example)
Details
@github-actions
test (macos-latest)
Details
@github-actions
test (macos-latest)
Details
@github-actions
check-errors (commands/daemon/term_example)
Details
@github-actions
check-errors (docsgen)
Details
@github-actions
check-outdated (./)
Details
@github-actions
check
Details
@github-actions
checksums artifact
Details
@github-actions
check-outdated (arduino/discovery/discovery_client)
Details
@github-actions
Linux_X86-32 artifact
Details
@github-actions
check-outdated (client_example)
Details
@github-actions
Linux_X86-64 artifact
Details
@github-actions
check-outdated (commands/daemon/term_example)
Details
@github-actions
Linux_ARM64 artifact
Details
@github-actions
check-outdated (docsgen)
Details
@github-actions
Linux_ARMv6 artifact
Details
@github-actions
Linux_ARMv7 artifact
Details
@github-actions
macOS_64 artifact
Details
@github-actions
Windows_X86-32 artifact
Details
@github-actions
Windows_X86-64 artifact
Details
@github-actions
check-style (./)
Details
@github-actions
check-formatting
Details
@github-actions
clean
Details
@github-actions
check-style (arduino/discovery/discovery_client)
Details
@github-actions
check-style (client_example)
Details
@github-actions
check-style (commands/daemon/term_example)
Details
@github-actions
check-style (docsgen)
Details
@github-actions
check-formatting (./)
Details
@github-actions
check-formatting (arduino/discovery/discovery_client)
Details
@github-actions
check-formatting (client_example)
Details
@github-actions
check-formatting (commands/daemon/term_example)
Details
@github-actions
check-formatting (docsgen)
Details
@github-actions
check-config (./)
Details
@github-actions
check-config (arduino/discovery/discovery_client)
Details
@github-actions
check-config (client_example)
Details
@github-actions
check-config (commands/daemon/term_example)
Details
@github-actions
check-config (docsgen)
Details
@github-actions
build (arduino/discovery/discovery_client)
Details
@github-actions
build (client_example)
Details
@github-actions
build (commands/daemon/term_example)
Details
license/cla Contributor License Agreement is signed.
Details
@cmaglie cmaglie deleted the cmaglie:proper-errors branch Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants