From d0402e009348ea7706501c7e0fee56bb57072a19 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 29 Nov 2016 18:16:57 -0800 Subject: [PATCH 1/5] Node: correctly bubble up errors caused by non-serializable writes --- src/client.js | 13 ++++++++++++- src/server.js | 7 ++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/client.js b/src/client.js index f75f951e..56aa8907 100644 --- a/src/client.js +++ b/src/client.js @@ -99,7 +99,18 @@ function ClientWritableStream(call, serialize) { function _write(chunk, encoding, callback) { /* jshint validthis: true */ var batch = {}; - var message = this.serialize(chunk); + var message; + try { + message = this.serialize(chunk); + } catch (e) { + /* Sending this error to the server and emitting it immediately on the + client may put the call in a slightly weird state on the client side, + but passing an object that causes a serialization failure is a misuse + of the API anyway, so that's OK. The primary purpose here is to give the + programmer a useful error and to stop the stream properly */ + this.call.cancelWithStatus(grpc.status.INTERNAL, "Serialization failure"); + callback(e); + } if (_.isFinite(encoding)) { /* Attach the encoding if it is a finite number. This is the closest we * can get to checking that it is valid flags */ diff --git a/src/server.js b/src/server.js index b3b41496..bd0a5122 100644 --- a/src/server.js +++ b/src/server.js @@ -278,7 +278,12 @@ function _write(chunk, encoding, callback) { (new Metadata())._getCoreRepresentation(); this.call.metadataSent = true; } - var message = this.serialize(chunk); + var message; + try { + message = this.serialize(chunk); + } catch (e) { + callback(e); + } if (_.isFinite(encoding)) { /* Attach the encoding if it is a finite number. This is the closest we * can get to checking that it is valid flags */ From 15d48751381d56b255385b0a8247360c71f342d0 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 1 Dec 2016 10:30:35 -0800 Subject: [PATCH 2/5] Also propagate serialization errors in unary server responses --- src/server.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/server.js b/src/server.js index bd0a5122..51bb99ee 100644 --- a/src/server.js +++ b/src/server.js @@ -127,7 +127,13 @@ function sendUnaryResponse(call, value, serialize, metadata, flags) { (new Metadata())._getCoreRepresentation(); call.metadataSent = true; } - var message = serialize(value); + var message; + try { + message = serialize(value); + } catch (e) { + e.code = grpc.status.INTERNAL; + handleError(e); + } message.grpcWriteFlags = flags; end_batch[grpc.opType.SEND_MESSAGE] = message; end_batch[grpc.opType.SEND_STATUS_FROM_SERVER] = status; @@ -282,7 +288,9 @@ function _write(chunk, encoding, callback) { try { message = this.serialize(chunk); } catch (e) { + e.code = grpc.status.INTERNAL; callback(e); + return; } if (_.isFinite(encoding)) { /* Attach the encoding if it is a finite number. This is the closest we From 5c8df435fa6d168ff09d653f6897d22ed462bacb Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 1 Dec 2016 10:59:52 -0800 Subject: [PATCH 3/5] Add missing return --- src/server.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.js b/src/server.js index 51bb99ee..da9c6b2d 100644 --- a/src/server.js +++ b/src/server.js @@ -133,6 +133,7 @@ function sendUnaryResponse(call, value, serialize, metadata, flags) { } catch (e) { e.code = grpc.status.INTERNAL; handleError(e); + return; } message.grpcWriteFlags = flags; end_batch[grpc.opType.SEND_MESSAGE] = message; From 0ea53cccdd9fefac4e284f88a4e13c6a1f1bd658 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 6 Dec 2016 14:18:21 -0800 Subject: [PATCH 4/5] Perform quit operations in a useful order in Node perf tests --- performance/worker_service_impl.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/performance/worker_service_impl.js b/performance/worker_service_impl.js index 3f317f64..38888a72 100644 --- a/performance/worker_service_impl.js +++ b/performance/worker_service_impl.js @@ -55,9 +55,8 @@ module.exports = function WorkerServiceImpl(benchmark_impl, server) { } this.quitWorker = function quitWorker(call, callback) { - server.tryShutdown(function() { - callback(null, {}); - }); + callback(null, {}); + server.tryShutdown(function() {}); }; this.runClient = function runClient(call) { From 6962aed6fa22a3f607c9a1fe5218b835a083e5c2 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 13 Dec 2016 18:05:33 -0800 Subject: [PATCH 5/5] Make event order consistent, and make 'end' and 'error' mutually exclusive --- src/client.js | 13 ++++++++----- test/surface_test.js | 8 ++++---- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/client.js b/src/client.js index 9c1562e8..0f85f2c6 100644 --- a/src/client.js +++ b/src/client.js @@ -184,14 +184,15 @@ function _emitStatusIfDone() { } else { status = this.received_status; } - this.emit('status', status); - if (status.code !== grpc.status.OK) { + if (status.code === grpc.status.OK) { + this.push(null); + } else { var error = new Error(status.details); error.code = status.code; error.metadata = status.metadata; this.emit('error', error); - return; } + this.emit('status', status); } } @@ -224,9 +225,11 @@ function _read(size) { } catch (e) { self._readsDone({code: grpc.status.INTERNAL, details: 'Failed to parse server response'}); + return; } if (data === null) { self._readsDone(); + return; } if (self.push(deserialized) && data !== null) { var read_batch = {}; @@ -396,6 +399,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { var status = response.status; var error; var deserialized; + emitter.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); if (status.code === grpc.status.OK) { if (err) { // Got a batch error, but OK status. Something went wrong @@ -423,8 +428,6 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { args.callback(null, deserialized); } emitter.emit('status', status); - emitter.emit('metadata', Metadata._fromCoreRepresentation( - response.metadata)); }); return emitter; } diff --git a/test/surface_test.js b/test/surface_test.js index d8b36dc5..2a42dd5d 100644 --- a/test/surface_test.js +++ b/test/surface_test.js @@ -179,8 +179,8 @@ describe('Server.prototype.addProtoService', function() { call.on('data', function(value) { assert.fail('No messages expected'); }); - call.on('status', function(status) { - assert.strictEqual(status.code, grpc.status.UNIMPLEMENTED); + call.on('error', function(err) { + assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED); done(); }); }); @@ -189,8 +189,8 @@ describe('Server.prototype.addProtoService', function() { call.on('data', function(value) { assert.fail('No messages expected'); }); - call.on('status', function(status) { - assert.strictEqual(status.code, grpc.status.UNIMPLEMENTED); + call.on('error', function(err) { + assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED); done(); }); call.end();