Alternative format for api with error handling

[sampsongao]

As I start to update nanomsg code to use error handling api, I found that it is quite hard to use.

For example:

NAPI_METHOD(Shutdown) {
  napi_value args[2];
  napi_get_cb_args(env, info, args, 2);

  int s = napi_get_value_int64(env, args[0], &s);
  int how = TRY_CATCH(napi_get_value_int64(env, args[1], &how);

  napi_value ret = napi_create_number(env, nn_shutdown(s, how), &ret);
  napi_set_return_value(env, info, ret);
}

will be converted to the following:

#define CHECK_STATUS                         \
  if (status != napi_ok) {                   \
    return;                                  \
  }

NAPI_METHOD(Shutdown) {
  napi_status status;
  napi_value args[2];
  status = napi_get_cb_args(env, info, args, 2);
  CHECK_STATUS;

  int s;
  status = napi_get_value_int64(env, args[0], &s);
  CHECK_STATUS;
  int how;
  status = napi_get_value_int64(env, args[1], &how);
  CHECK_STATUS;

  napi_value ret;
  status = napi_create_number(env, nn_shutdown(s, how), &ret);
  CHECK_STATUS;
  status = napi_set_return_value(env, info, ret);
  CHECK_STATUS;
}

So I think alternatively we should let developer to pass in napi_status to get the status, e.g.

napi_value napi_create_number(napi_env env, int num, napi_status* status);

This way, the above code can be converted as:

#define CHECK_STATUS                         \
  if (status != napi_ok) {                   \
    return;                                  \
  }

NAPI_METHOD(Shutdown) {
  napi_status status;
  napi_value args[2];
  napi_get_cb_args(env, info, args, 2);

  int s = napi_get_value_int64(env, args[0], &status);
  CHECK_STATUS;
  int how = napi_get_value_int64(env, args[1], &status)
  CHECK_STATUS;

  napi_value ret = napi_create_number(env, nn_shutdown(s, how), &status);
  CHECK_STATUS;
  napi_set_return_value(env, info, ret, &status);
  CHECK_STATUS;
}

This allows declaration on same line and nested calls where possible. I think this can be more flexible.
@aruneshchandra, @boingoing, @jasongin, @gabrielschulhof Please comment about what you think.

[jasongin]

During the design discussions I had brought up use of out error parameters for consideration also (inspired by Objective-C use of NSError). It can have the additional benefit that you don't need to call a separate get-error-info function to get more details, since the out parameter can be a struct pointer instead of just an integer code. I don't think we discussed it much in the online meetings, but Taylor and I did talk about it in person.

However, I actually think now that returning a status code via the function return value and results via out parameters, as we have implemented, is more traditional for flat C APIs and likely to be more familiar to users of the API.

As for the usability issue that you point out, I think the best solution is C++ wrapper classes that make the C APIs much easier to consume. (I had already opened issue #53 for that.) Among other things they would convert error codes into C++ exceptions. I've been meaning to prototype a C++ object model, but haven't had time yet.

[mhdawson]

The thing that jumps out is that its going to be quite a bit more verbose having to declare the return value on a separate line, its also worrying if it is going to make the porting harder.

I do agree that it is more traditional to have the return code be the return value as opposed to the in/out.

@sampsongao from what you looked at so far do you have an guestimate about how many more lines of code we would have for nanomsg using one approach versus the other, and similarly how much more effort it might be to complete the port ?

[jasongin]

The thing that jumps out is that its going to be quite a bit more verbose having to declare the return value on a separate line, its also worrying if it is going to make the porting harder.

A C++ wrapper object model could make everything much less verbose, and should make porting much easier since the semantics can be more similar to the V8 C++ object model. Unfortunately we don't have such a thing ready for our initial porting efforts.

[boingoing]

I tend to agree all-around. This flat API is pretty verbose to write but we are expecting to hide most of that behind a C++ wrapper with much more convenient semantics like converting failures into exceptions. That won't be ready for this milestone, though, for sure. After porting leveldown and nanomsg we'll at least have a better handle of the pain points, if that's reassuring.

[aruneshchandra]

We are prepping for a public release of sorts if accepted for Node v8.0. And it appears that we will need the C++ sugar before we go out and ask people to adopt this. Looks like a Milestone 5 work item to me.

[aruneshchandra]

Added Issue #53 C++ Helper Classes to Milestone 5. Let's talk about this on Friday.

[jasongin]

With the C++ wrapper, the example (2nd snippet in first comment in this thread) becomes much simpler:

void Shutdown(const Napi::CallbackInfo& info) {
  int s = info[0]->As<Napi::Number>();
  int how = info[1]->As<Napi::Number>();
  return Napi::Number::New(info.Env(), nn_shutdown(s, how));
}

It may look like that C++ code has no error-handling, but actually it does: the wrapper may throw C++ exceptions, that are automatically re-thrown as JavaScript exceptions if not handled.

[jasongin]

Closing since this is addressed by the C++ wrapper classes.