Refactored some C++ code to improve code reuse

8 files changed, 107 insertions, 131 deletions

diff --git a/src/node/ext/call.cc b/src/node/ext/call.cc
index fcad62261f..fe11905109 100644
--- a/src/node/ext/call.cc
+++ b/src/node/ext/call.cc
@@ -83,6 +83,18 @@ using v8::Value;
 Callback *Call::constructor;
 Persistent<FunctionTemplate> Call::fun_tpl;
 
+/**
+ * Helper function for throwing errors with a grpc_call_error value.
+ * Modified from the answer by Gus Goose to
+ * http://stackoverflow.com/questions/31794200.
+ */
+Local<Value> nanErrorWithCode(const char *msg, grpc_call_error code) {
+  EscapableHandleScope scope;
+  Local<Object> err = Nan::Error(msg).As<Object>();
+  Nan::Set(err, Nan::New("code").ToLocalChecked(), Nan::New<Uint32>(code));
+  return scope.Escape(err);
+}
+
 bool EndsWith(const char *str, const char *substr) {
   return strcmp(str+strlen(str)-strlen(substr), substr) == 0;
 }
diff --git a/src/node/ext/call.h b/src/node/ext/call.h
index dd6c38e4f8..1e3c3ba18d 100644
--- a/src/node/ext/call.h
+++ b/src/node/ext/call.h
@@ -53,18 +53,7 @@ using std::shared_ptr;
 
 typedef Nan::Persistent<v8::Value, Nan::CopyablePersistentTraits<v8::Value>> PersistentValue;
 
-/**
- * Helper function for throwing errors with a grpc_call_error value.
- * Modified from the answer by Gus Goose to
- * http://stackoverflow.com/questions/31794200.
- */
-inline v8::Local<v8::Value> nanErrorWithCode(const char *msg,
-                                             grpc_call_error code) {
-  Nan::EscapableHandleScope scope;
-    v8::Local<v8::Object> err = Nan::Error(msg).As<v8::Object>();
-    Nan::Set(err, Nan::New("code").ToLocalChecked(), Nan::New<v8::Uint32>(code));
-    return scope.Escape(err);
-}
+v8::Local<v8::Value> nanErrorWithCode(const char *msg, grpc_call_error code);
 
 v8::Local<v8::Value> ParseMetadata(const grpc_metadata_array *metadata_array);
 
diff --git a/src/node/ext/call_credentials.cc b/src/node/ext/call_credentials.cc
index 839bb567e4..ff16a1f122 100644
--- a/src/node/ext/call_credentials.cc
+++ b/src/node/ext/call_credentials.cc
@@ -126,16 +126,9 @@ NAN_METHOD(CallCredentials::New) {
     info.GetReturnValue().Set(info.This());
     return;
   } else {
-    const int argc = 1;
-    Local<Value> argv[argc] = {info[0]};
-    MaybeLocal<Object> maybe_instance = constructor->GetFunction()->NewInstance(
-        argc, argv);
-    if (maybe_instance.IsEmpty()) {
-      // There's probably a pending exception
-      return;
-    } else {
-      info.GetReturnValue().Set(maybe_instance.ToLocalChecked());
-    }
+    // This should never be called directly
+    return Nan::ThrowTypeError(
+        "CallCredentials can only be created with the provided functions");
   }
 }
 
diff --git a/src/node/ext/channel.cc b/src/node/ext/channel.cc
index a328c01713..584a0cf8ab 100644
--- a/src/node/ext/channel.cc
+++ b/src/node/ext/channel.cc
@@ -71,6 +71,72 @@ using v8::Value;
 Callback *Channel::constructor;
 Persistent<FunctionTemplate> Channel::fun_tpl;
 
+bool ParseChannelArgs(Local<Value> args_val,
+                      grpc_channel_args **channel_args_ptr) {
+  if (args_val->IsUndefined() || args_val->IsNull()) {
+    *channel_args_ptr = NULL;
+    return true;
+  }
+  if (!args_val->IsObject()) {
+    *channel_args_ptr = NULL;
+    return false;
+  }
+  grpc_channel_args *channel_args = reinterpret_cast<grpc_channel_args*>(
+      malloc(sizeof(channel_args)));
+  *channel_args_ptr = channel_args;
+  Local<Object> args_hash = Nan::To<Object>(args_val).ToLocalChecked();
+  Local<Array> keys = Nan::GetOwnPropertyNames(args_hash).ToLocalChecked();
+  channel_args->num_args = keys->Length();
+  channel_args->args = reinterpret_cast<grpc_arg *>(
+      calloc(channel_args->num_args, sizeof(grpc_arg)));
+  for (unsigned int i = 0; i < channel_args->num_args; i++) {
+    Local<Value> key = Nan::Get(keys, i).ToLocalChecked();
+    Utf8String key_str(key);
+    if (*key_str == NULL) {
+      // Key string onversion failed
+      return false;
+    }
+    Local<Value> value = Nan::Get(args_hash, key).ToLocalChecked();
+    if (value->IsInt32()) {
+      channel_args->args[i].type = GRPC_ARG_INTEGER;
+      channel_args->args[i].value.integer = Nan::To<int32_t>(value).FromJust();
+    } else if (value->IsString()) {
+      Utf8String val_str(value);
+      channel_args->args[i].type = GRPC_ARG_STRING;
+      channel_args->args[i].value.string = reinterpret_cast<char*>(
+          calloc(val_str.length() + 1,sizeof(char)));
+      memcpy(channel_args->args[i].value.string,
+             *val_str, val_str.length() + 1);
+    } else {
+      // The value does not match either of the accepted types
+      return false;
+    }
+    channel_args->args[i].key = reinterpret_cast<char*>(
+        calloc(key_str.length() + 1, sizeof(char)));
+    memcpy(channel_args->args[i].key, *key_str, key_str.length() + 1);
+  }
+  return true;
+}
+
+void DeallocateChannelArgs(grpc_channel_args *channel_args) {
+  if (channel_args == NULL) {
+    return;
+  }
+  for (size_t i = 0; i < channel_args->num_args; i++) {
+    if (channel_args->args[i].key == NULL) {
+      /* NULL key implies that this argument and all subsequent arguments failed
+       * to parse */
+      break;
+    }
+    free(channel_args->args[i].key);
+    if (channel_args->args[i].type == GRPC_ARG_STRING) {
+      free(channel_args->args[i].value.string);
+    }
+  }
+  free(channel_args->args);
+  free(channel_args);
+}
+
 Channel::Channel(grpc_channel *channel) : wrapped_channel(channel) {}
 
 Channel::~Channel() {
@@ -119,49 +185,11 @@ NAN_METHOD(Channel::New) {
     ChannelCredentials *creds_object = ObjectWrap::Unwrap<ChannelCredentials>(
         Nan::To<Object>(info[1]).ToLocalChecked());
     creds = creds_object->GetWrappedCredentials();
-    grpc_channel_args *channel_args_ptr;
-    if (info[2]->IsUndefined()) {
-      channel_args_ptr = NULL;
-      wrapped_channel = grpc_insecure_channel_create(*host, NULL, NULL);
-    } else if (info[2]->IsObject()) {
-      Local<Object> args_hash = Nan::To<Object>(info[2]).ToLocalChecked();
-      Local<Array> keys(Nan::GetOwnPropertyNames(args_hash).ToLocalChecked());
-      grpc_channel_args channel_args;
-      channel_args.num_args = keys->Length();
-      channel_args.args = reinterpret_cast<grpc_arg *>(
-          calloc(channel_args.num_args, sizeof(grpc_arg)));
-      /* These are used to keep all strings until then end of the block, then
-         destroy them */
-      std::vector<Nan::Utf8String *> key_strings(keys->Length());
-      std::vector<Nan::Utf8String *> value_strings(keys->Length());
-      for (unsigned int i = 0; i < channel_args.num_args; i++) {
-        MaybeLocal<String> maybe_key = Nan::To<String>(
-            Nan::Get(keys, i).ToLocalChecked());
-        if (maybe_key.IsEmpty()) {
-          free(channel_args.args);
-          return Nan::ThrowTypeError("Arg keys must be strings");
-        }
-        Local<String> current_key = maybe_key.ToLocalChecked();
-        Local<Value> current_value = Nan::Get(args_hash,
-                                               current_key).ToLocalChecked();
-        key_strings[i] = new Nan::Utf8String(current_key);
-        channel_args.args[i].key = **key_strings[i];
-        if (current_value->IsInt32()) {
-          channel_args.args[i].type = GRPC_ARG_INTEGER;
-          channel_args.args[i].value.integer = Nan::To<int32_t>(
-              current_value).FromJust();
-        } else if (current_value->IsString()) {
-          channel_args.args[i].type = GRPC_ARG_STRING;
-          value_strings[i] = new Nan::Utf8String(current_value);
-          channel_args.args[i].value.string = **value_strings[i];
-        } else {
-          free(channel_args.args);
-          return Nan::ThrowTypeError("Arg values must be strings");
-        }
-      }
-      channel_args_ptr = &channel_args;
-    } else {
-      return Nan::ThrowTypeError("Channel expects a string and an object");
+    grpc_channel_args *channel_args_ptr = NULL;
+    if (!ParseChannelArgs(info[2], &channel_args_ptr)) {
+      DeallocateChannelArgs(channel_args_ptr);
+      return Nan::ThrowTypeError("Channel options must be an object with "
+                                 "string keys and integer or string values");
     }
     if (creds == NULL) {
       wrapped_channel = grpc_insecure_channel_create(*host, channel_args_ptr,
@@ -170,9 +198,7 @@ NAN_METHOD(Channel::New) {
       wrapped_channel =
           grpc_secure_channel_create(creds, *host, channel_args_ptr, NULL);
     }
-    if (channel_args_ptr != NULL) {
-      free(channel_args_ptr->args);
-    }
+    DeallocateChannelArgs(channel_args_ptr);
     Channel *channel = new Channel(wrapped_channel);
     channel->Wrap(info.This());
     info.GetReturnValue().Set(info.This());
diff --git a/src/node/ext/channel.h b/src/node/ext/channel.h
index 0062fd03f4..9ec28e15af 100644
--- a/src/node/ext/channel.h
+++ b/src/node/ext/channel.h
@@ -41,6 +41,11 @@
 namespace grpc {
 namespace node {
 
+bool ParseChannelArgs(v8::Local<v8::Value> args_val,
+                      grpc_channel_args **channel_args_ptr);
+
+void DeallocateChannelArgs(grpc_channel_args *channel_args);
+
 /* Wrapper class for grpc_channel structs */
 class Channel : public Nan::ObjectWrap {
  public:
diff --git a/src/node/ext/channel_credentials.cc b/src/node/ext/channel_credentials.cc
index 3d47ff293d..7ca3b9816c 100644
--- a/src/node/ext/channel_credentials.cc
+++ b/src/node/ext/channel_credentials.cc
@@ -127,16 +127,9 @@ NAN_METHOD(ChannelCredentials::New) {
     info.GetReturnValue().Set(info.This());
     return;
   } else {
-    const int argc = 1;
-    Local<Value> argv[argc] = {info[0]};
-    MaybeLocal<Object> maybe_instance = constructor->GetFunction()->NewInstance(
-        argc, argv);
-    if (maybe_instance.IsEmpty()) {
-      // There's probably a pending exception
-      return;
-    } else {
-      info.GetReturnValue().Set(maybe_instance.ToLocalChecked());
-    }
+    // This should never be called directly
+    return Nan::ThrowTypeError(
+        "ChannelCredentials can only be created with the provided functions");
   }
 }
 
diff --git a/src/node/ext/server.cc b/src/node/ext/server.cc
index 87363fc446..b9e1fe9160 100644
--- a/src/node/ext/server.cc
+++ b/src/node/ext/server.cc
@@ -182,49 +182,14 @@ NAN_METHOD(Server::New) {
   }
   grpc_server *wrapped_server;
   grpc_completion_queue *queue = CompletionQueueAsyncWorker::GetQueue();
-  if (info[0]->IsUndefined()) {
-    wrapped_server = grpc_server_create(NULL, NULL);
-  } else if (info[0]->IsObject()) {
-    Local<Object> args_hash = Nan::To<Object>(info[0]).ToLocalChecked();
-    Local<Array> keys = Nan::GetOwnPropertyNames(args_hash).ToLocalChecked();
-    grpc_channel_args channel_args;
-    channel_args.num_args = keys->Length();
-    channel_args.args = reinterpret_cast<grpc_arg *>(
-        calloc(channel_args.num_args, sizeof(grpc_arg)));
-    /* These are used to keep all strings until then end of the block, then
-       destroy them */
-    std::vector<Utf8String *> key_strings(keys->Length());
-    std::vector<Utf8String *> value_strings(keys->Length());
-    for (unsigned int i = 0; i < channel_args.num_args; i++) {
-      MaybeLocal<String> maybe_key = Nan::To<String>(
-          Nan::Get(keys, i).ToLocalChecked());
-      if (maybe_key.IsEmpty()) {
-        free(channel_args.args);
-        return Nan::ThrowTypeError("Arg keys must be strings");
-      }
-      Local<String> current_key = maybe_key.ToLocalChecked();
-      Local<Value> current_value = Nan::Get(args_hash,
-                                             current_key).ToLocalChecked();
-      key_strings[i] = new Utf8String(current_key);
-      channel_args.args[i].key = **key_strings[i];
-      if (current_value->IsInt32()) {
-        channel_args.args[i].type = GRPC_ARG_INTEGER;
-        channel_args.args[i].value.integer = Nan::To<int32_t>(
-            current_value).FromJust();
-      } else if (current_value->IsString()) {
-        channel_args.args[i].type = GRPC_ARG_STRING;
-        value_strings[i] = new Utf8String(current_value);
-        channel_args.args[i].value.string = **value_strings[i];
-      } else {
-        free(channel_args.args);
-        return Nan::ThrowTypeError("Arg values must be strings");
-      }
-    }
-    wrapped_server = grpc_server_create(&channel_args, NULL);
-    free(channel_args.args);
-  } else {
-    return Nan::ThrowTypeError("Server expects an object");
+  grpc_channel_args *channel_args;
+  if (!ParseChannelArgs(info[0], &channel_args)) {
+    DeallocateChannelArgs(channel_args);
+    return Nan::ThrowTypeError("Server options must be an object with "
+                               "string keys and integer or string values");
   }
+  wrapped_server = grpc_server_create(channel_args, NULL);
+  DeallocateChannelArgs(channel_args);
   grpc_server_register_completion_queue(wrapped_server, queue, NULL);
   Server *server = new Server(wrapped_server);
   server->Wrap(info.This());
diff --git a/src/node/ext/server_credentials.cc b/src/node/ext/server_credentials.cc
index 5e922bd877..e6c55e263c 100644
--- a/src/node/ext/server_credentials.cc
+++ b/src/node/ext/server_credentials.cc
@@ -117,7 +117,7 @@ NAN_METHOD(ServerCredentials::New) {
   if (info.IsConstructCall()) {
     if (!info[0]->IsExternal()) {
       return Nan::ThrowTypeError(
-          "ServerCredentials can only be created with the provide functions");
+          "ServerCredentials can only be created with the provided functions");
     }
     Local<External> ext = info[0].As<External>();
     grpc_server_credentials *creds_value =
@@ -126,16 +126,9 @@ NAN_METHOD(ServerCredentials::New) {
     credentials->Wrap(info.This());
     info.GetReturnValue().Set(info.This());
   } else {
-    const int argc = 1;
-    Local<Value> argv[argc] = {info[0]};
-    MaybeLocal<Object> maybe_instance = constructor->GetFunction()->NewInstance(
-        argc, argv);
-    if (maybe_instance.IsEmpty()) {
-      // There's probably a pending exception
-      return;
-    } else {
-      info.GetReturnValue().Set(maybe_instance.ToLocalChecked());
-    }
+    // This should never be called directly
+    return Nan::ThrowTypeError(
+        "ServerCredentials can only be created with the provided functions");
   }
 }