Merge pull request #11304 from murgatroid99/node_call_lifetime_fixes

Add more null checks to call methods

4 files changed, 43 insertions, 4 deletions

diff --git a/src/node/ext/call.cc b/src/node/ext/call.cc
index 2cc9f63b65..e2185da1c5 100644
--- a/src/node/ext/call.cc
+++ b/src/node/ext/call.cc
@@ -562,10 +562,12 @@ void Call::DestroyCall() {
 Call::Call(grpc_call *call) : wrapped_call(call),
                               pending_batches(0),
                               has_final_op_completed(false) {
+  peer = grpc_call_get_peer(call);
 }
 
 Call::~Call() {
   DestroyCall();
+  gpr_free(peer);
 }
 
 void Call::Init(Local<Object> exports) {
@@ -794,6 +796,11 @@ NAN_METHOD(Call::Cancel) {
     return Nan::ThrowTypeError("cancel can only be called on Call objects");
   }
   Call *call = ObjectWrap::Unwrap<Call>(info.This());
+  if (call->wrapped_call == NULL) {
+    /* Cancel is supposed to be idempotent. If the call has already finished,
+     * cancel should just complete silently */
+    return;
+  }
   grpc_call_error error = grpc_call_cancel(call->wrapped_call, NULL);
   if (error != GRPC_CALL_OK) {
     return Nan::ThrowError(nanErrorWithCode("cancel failed", error));
@@ -814,6 +821,11 @@ NAN_METHOD(Call::CancelWithStatus) {
         "cancelWithStatus's second argument must be a string");
   }
   Call *call = ObjectWrap::Unwrap<Call>(info.This());
+  if (call->wrapped_call == NULL) {
+    /* Cancel is supposed to be idempotent. If the call has already finished,
+     * cancel should just complete silently */
+    return;
+  }
   grpc_status_code code = static_cast<grpc_status_code>(
       Nan::To<uint32_t>(info[0]).FromJust());
   if (code == GRPC_STATUS_OK) {
@@ -830,9 +842,7 @@ NAN_METHOD(Call::GetPeer) {
     return Nan::ThrowTypeError("getPeer can only be called on Call objects");
   }
   Call *call = ObjectWrap::Unwrap<Call>(info.This());
-  char *peer = grpc_call_get_peer(call->wrapped_call);
-  Local<Value> peer_value = Nan::New(peer).ToLocalChecked();
-  gpr_free(peer);
+  Local<Value> peer_value = Nan::New(call->peer).ToLocalChecked();
   info.GetReturnValue().Set(peer_value);
 }
 
@@ -847,6 +857,10 @@ NAN_METHOD(Call::SetCredentials) {
         "setCredentials' first argument must be a CallCredentials");
   }
   Call *call = ObjectWrap::Unwrap<Call>(info.This());
+  if (call->wrapped_call == NULL) {
+    return Nan::ThrowError(
+        "Cannot set credentials on a call that has already started");
+  }
   CallCredentials *creds_object = ObjectWrap::Unwrap<CallCredentials>(
       Nan::To<Object>(info[0]).ToLocalChecked());
   grpc_call_credentials *creds = creds_object->GetWrappedCredentials();
diff --git a/src/node/ext/call.h b/src/node/ext/call.h
index 340e32682b..45b448e0b1 100644
--- a/src/node/ext/call.h
+++ b/src/node/ext/call.h
@@ -97,6 +97,7 @@ class Call : public Nan::ObjectWrap {
      call, this is GRPC_OP_RECV_STATUS_ON_CLIENT and for a server call, this
      is GRPC_OP_SEND_STATUS_FROM_SERVER */
   bool has_final_op_completed;
+  char *peer;
 };
 
 class Op {
diff --git a/src/node/test/common_test.js b/src/node/test/common_test.js
index b7c2c6a8d6..db80207e22 100644
--- a/src/node/test/common_test.js
+++ b/src/node/test/common_test.js
@@ -100,7 +100,6 @@ describe('Proto message long int serialize and deserialize', function() {
     var longNumDeserialize = deserializeCls(messages_proto.LongValues,
                                             num_options);
     var serialized = longSerialize({int_64: pos_value});
-    console.log(longDeserialize(serialized));
     assert.strictEqual(typeof longDeserialize(serialized).int_64, 'string');
     /* With the longsAsStrings option disabled, long values are represented as
      * objects with 3 keys: low, high, and unsigned */
diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js
index d2f0511af2..0696e7ae19 100644
--- a/src/node/test/surface_test.js
+++ b/src/node/test/surface_test.js
@@ -1110,6 +1110,18 @@ describe('Other conditions', function() {
         done();
       });
     });
+    it('after the call has fully completed', function(done) {
+      var peer;
+      var call = client.unary({error: false}, function(err, data) {
+        assert.ifError(err);
+        setImmediate(function() {
+          assert.strictEqual(peer, call.getPeer());
+          done();
+        });
+      });
+      peer = call.getPeer();
+      assert.strictEqual(typeof peer, 'string');
+    });
   });
 });
 describe('Call propagation', function() {
@@ -1352,4 +1364,17 @@ describe('Cancelling surface client', function() {
     });
     call.cancel();
   });
+  it('Should be idempotent', function(done) {
+    var call = client.div({'divisor': 0, 'dividend': 0}, function(err, resp) {
+      assert.strictEqual(err.code, surface_client.status.CANCELLED);
+      // Call asynchronously to try cancelling after call is fully completed
+      setImmediate(function() {
+        assert.doesNotThrow(function() {
+          call.cancel();
+        });
+        done();
+      });
+    });
+    call.cancel();
+  });
 });