From 9538b3d5a9d3732d3aab8c996fba9de5c0e61ef4 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 15 May 2017 14:39:05 -0700 Subject: [PATCH 1/6] Fix race between destroying call after status and handling write failure --- ext/call.cc | 8 ++++++++ src/client.js | 12 ++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/ext/call.cc b/ext/call.cc index 8877995f..2cc9f63b 100644 --- a/ext/call.cc +++ b/ext/call.cc @@ -721,6 +721,14 @@ NAN_METHOD(Call::StartBatch) { } Local callback_func = info[1].As(); Call *call = ObjectWrap::Unwrap(info.This()); + if (call->wrapped_call == NULL) { + /* This implies that the call has completed and has been destroyed. To emulate + * previous behavior, we should call the callback immediately with an error, + * as though the batch had failed in core */ + Local argv[] = {Nan::Error("The async function failed because the call has completed")}; + Nan::Call(callback_func, Nan::New(), 1, argv); + return; + } Local obj = Nan::To(info[0]).ToLocalChecked(); Local keys = Nan::GetOwnPropertyNames(obj).ToLocalChecked(); size_t nops = keys->Length(); diff --git a/src/client.js b/src/client.js index 1aaf35c1..aa818349 100644 --- a/src/client.js +++ b/src/client.js @@ -100,6 +100,12 @@ function _write(chunk, encoding, callback) { /* jshint validthis: true */ var batch = {}; var message; + var self = this; + if (this.writeFailed) { + /* Once a write fails, just call the callback immediately to let the caller + flush any pending writes. */ + callback(); + } try { message = this.serialize(chunk); } catch (e) { @@ -119,8 +125,10 @@ function _write(chunk, encoding, callback) { batch[grpc.opType.SEND_MESSAGE] = message; this.call.startBatch(batch, function(err, event) { if (err) { - // Something has gone wrong. Stop writing by failing to call callback - return; + /* Assume that the call is complete and that writing failed because a + status was received. In that case, set a flag to discard all future + writes */ + self.writeFailed = true; } callback(); }); From d82c77ef02dbca115f9be8b4a2b084b0135ca303 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 16 May 2017 12:53:57 -0700 Subject: [PATCH 2/6] Change write callback to asynchronous to avoid recursion --- src/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client.js b/src/client.js index aa818349..2073d3ee 100644 --- a/src/client.js +++ b/src/client.js @@ -104,7 +104,7 @@ function _write(chunk, encoding, callback) { if (this.writeFailed) { /* Once a write fails, just call the callback immediately to let the caller flush any pending writes. */ - callback(); + setImmediate(callback); } try { message = this.serialize(chunk); From 7b9d7663e83c5bfa3fa925b225376fe44f5119f9 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Thu, 18 May 2017 11:49:15 -0700 Subject: [PATCH 3/6] Bump to version 1.3.4 --- health_check/package.json | 4 ++-- tools/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/health_check/package.json b/health_check/package.json index fe36fb26..6329cd9c 100644 --- a/health_check/package.json +++ b/health_check/package.json @@ -1,6 +1,6 @@ { "name": "grpc-health-check", - "version": "1.3.3", + "version": "1.3.4", "author": "Google Inc.", "description": "Health check service for use with gRPC", "repository": { @@ -15,7 +15,7 @@ } ], "dependencies": { - "grpc": "^1.3.3", + "grpc": "^1.3.4", "lodash": "^3.9.3", "google-protobuf": "^3.0.0" }, diff --git a/tools/package.json b/tools/package.json index caf2b5a5..355ae2fd 100644 --- a/tools/package.json +++ b/tools/package.json @@ -1,6 +1,6 @@ { "name": "grpc-tools", - "version": "1.3.3", + "version": "1.3.4", "author": "Google Inc.", "description": "Tools for developing with gRPC on Node.js", "homepage": "http://www.grpc.io/", From d8e9234f864117b97fe28b9bf68bfad3e248eef9 Mon Sep 17 00:00:00 2001 From: Ken Payson Date: Fri, 19 May 2017 17:47:49 -0700 Subject: [PATCH 4/6] Update version to 1.3.5 --- health_check/package.json | 4 ++-- tools/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/health_check/package.json b/health_check/package.json index 6329cd9c..2fdcb127 100644 --- a/health_check/package.json +++ b/health_check/package.json @@ -1,6 +1,6 @@ { "name": "grpc-health-check", - "version": "1.3.4", + "version": "1.3.5", "author": "Google Inc.", "description": "Health check service for use with gRPC", "repository": { @@ -15,7 +15,7 @@ } ], "dependencies": { - "grpc": "^1.3.4", + "grpc": "^1.3.5", "lodash": "^3.9.3", "google-protobuf": "^3.0.0" }, diff --git a/tools/package.json b/tools/package.json index 355ae2fd..cabd4030 100644 --- a/tools/package.json +++ b/tools/package.json @@ -1,6 +1,6 @@ { "name": "grpc-tools", - "version": "1.3.4", + "version": "1.3.5", "author": "Google Inc.", "description": "Tools for developing with gRPC on Node.js", "homepage": "http://www.grpc.io/", From bbd820f138a2bf64194e6692837e764a8844b3a1 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 23 May 2017 22:48:45 +0200 Subject: [PATCH 5/6] bump version to 1.3.6 --- health_check/package.json | 4 ++-- tools/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/health_check/package.json b/health_check/package.json index 2fdcb127..f5700206 100644 --- a/health_check/package.json +++ b/health_check/package.json @@ -1,6 +1,6 @@ { "name": "grpc-health-check", - "version": "1.3.5", + "version": "1.3.6", "author": "Google Inc.", "description": "Health check service for use with gRPC", "repository": { @@ -15,7 +15,7 @@ } ], "dependencies": { - "grpc": "^1.3.5", + "grpc": "^1.3.6", "lodash": "^3.9.3", "google-protobuf": "^3.0.0" }, diff --git a/tools/package.json b/tools/package.json index cabd4030..59397a5c 100644 --- a/tools/package.json +++ b/tools/package.json @@ -1,6 +1,6 @@ { "name": "grpc-tools", - "version": "1.3.5", + "version": "1.3.6", "author": "Google Inc.", "description": "Tools for developing with gRPC on Node.js", "homepage": "http://www.grpc.io/", From 46b78f5fe3a3893d0813ae8b4a105dcc03f60671 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 25 May 2017 10:55:53 -0700 Subject: [PATCH 6/6] Add more null checks to call methods --- ext/call.cc | 20 +++++++++++++++++--- ext/call.h | 1 + test/common_test.js | 1 - test/surface_test.js | 25 +++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/ext/call.cc b/ext/call.cc index 2cc9f63b..e2185da1 100644 --- a/ext/call.cc +++ b/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 exports) { @@ -794,6 +796,11 @@ NAN_METHOD(Call::Cancel) { return Nan::ThrowTypeError("cancel can only be called on Call objects"); } Call *call = ObjectWrap::Unwrap(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(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( Nan::To(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(info.This()); - char *peer = grpc_call_get_peer(call->wrapped_call); - Local peer_value = Nan::New(peer).ToLocalChecked(); - gpr_free(peer); + Local 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(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( Nan::To(info[0]).ToLocalChecked()); grpc_call_credentials *creds = creds_object->GetWrappedCredentials(); diff --git a/ext/call.h b/ext/call.h index 340e3268..45b448e0 100644 --- a/ext/call.h +++ b/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/test/common_test.js b/test/common_test.js index b7c2c6a8..db80207e 100644 --- a/test/common_test.js +++ b/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/test/surface_test.js b/test/surface_test.js index d2f0511a..0696e7ae 100644 --- a/test/surface_test.js +++ b/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(); + }); });