Xud: Consistent gRPC error handling & responses

[sangaman]

Currently there's a bit of inconsistency with how our gRPC service deals with failures and with responses that essentially amount to indicating success or failure.

For example, Connect, Disconnect, and Shutdown return strings. CancelOrder returns a boolean.

PlaceOrder throws an error if the specified PairId does not exist, but CancelOrder does not (nor does it throw an error if the specified order_id does not exist).

I'm thinking that for all these sorts of calls where the response essentially indicates if the call succeeded or not, we have an empty response to indicate success and throw an error to indicate failure. This seems to be the same approach that lnd takes. We could also tweak xucli to print "success" anytime there is an empty response, as opposed to the less descriptive {} that would get printed otherwise.

I can definitely handle these changes, but I just wanted to see if anyone had any opinions before I start on it.

[programarivm]

Sorry that this may be duplicated at https://github.com/ExchangeUnion/xud/issues/322

I believe this is because of letting clients deal with errors and responses (or errors and messages). At this moment I am just wondering what is wrong with a grpc client using a Status object only instead of using two objects: Error and Response.

[sangaman]

I may not be following exactly what you mean, but the pattern of returning an error (or null if there was no error) and a response (or null if there was an error) just seems to be the standard pattern for grpc services and to me it seems like a fine approach. This issue isn't about whether we should discard grpc errors altogether, but just to make sure all calls deal with them in a consistent way.

[kilrau]

Sounds very reasonable @sangaman , let's do that!

Regarding printing: I'd prefer to print {SUCCESS} for an empty success response. Empty responses make sense to a service, but not so much to a human ;)

[offerm]

better not to invent the wheel here....

Why not to use the standard gRPC/Node way for sending errors?

See this for example http://avi.im/grpc-errors/#node

As a side note - I think XUD's gRPC server implementation isn't standard. It is related to the subject since a standard implementation makes it very easy (and standard) to handle errors. Will discuss offline with @sangaman

[programarivm]

The short answer: a status code is not an error.

I think node's error first callback paradigm doesn't fit very well with grpc's status codes -- most probably node's way of handling errors inspired a number of grpc libraries written in other programming languages.

For further information please visit:

• The Node.js Way - Understanding Error-First Callbacks
• What are the error conventions?

[sangaman]

@offerm I believe we are using the standard node approach, in GrpcService.ts internal errors are caught and passed to the callback with one of the grpc error status codes plus message. Definitely let me know what you mean by the grpc implementation being non standard, thanks.

[programarivm]

I like this C++ interface for grpc clients because it uses a status object and does not assume errors:

class GreeterClient {
 public:
  // ...
  std::string SayHello(const std::string& user) {
     // ...
  }

  std::string SayHelloAgain(const std::string& user) {
    // Follows the same pattern as SayHello.
    HelloRequest request;
    request.set_name(user);
    HelloReply reply;
    ClientContext context;

    // Here we can the stub's newly available method we just added.
    Status status = stub_->SayHelloAgain(&context, request, &reply);
    if (status.ok()) {
      return reply.message();
    } else {
      std::cout << status.error_code() << ": " << status.error_message()
                << std::endl;
      return "RPC failed";
    }
  }

I believe this is consistent with Status codes and their use in gRPC :

gRPC uses a set of well defined status codes as part of the RPC API. All RPCs started at a client return a status object composed of an integer code and a string message.

[sangaman]

From what I understand from talking to @offerm, his concern is over our separation of the generated grpc object types from the rest of our code, and the current lack of compiler error if we return an object from Service.ts that is not in our proto definition.

I'll open a separate issue to discuss that.

For this issue, I'm mainly focused on what we treat as grpc errors and what responses look like for calls that essentially have binary outcomes - call succeeded or call failed. Again I'm thinking of "call succeeded" being an empty response, and call failed being some sort of grpc error.

[kilrau]

Any agreements? @offerm @sangaman @programarivm

[programarivm]

Oh, it's been a while since I looked at the DDTD framework.

If my memory serves me, the API was being developed at that time and I wondered if it would respond with OK = 0 in order to use a naming convention like this one:

• GrpcStatus0.spec.ts
• GrpcStatus1.spec.ts
• ...

[sangaman]

426 closes this issue, it just needs to be reviewed. After that I think we'll be in a better place to go forward with @programarivm's PR and tests.

[programarivm]

Thanks for pinging. I'll try to look at the tests at this stage.

[sangaman]

@programarivm Great, I'll definitely take a look at your PR after you get a chance to revisit it, thank you.