From cb951f6c57c35d26ff8b643c4a498be397be6750 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 18 Aug 2015 17:38:11 -0700 Subject: [PATCH 01/12] Split server shutdown into tryShutdown and forceShutdown --- src/node/ext/server.cc | 50 +++++++++++++++------------- src/node/ext/server.h | 3 +- src/node/src/server.js | 21 ++++++++++-- src/node/test/call_test.js | 2 +- src/node/test/end_to_end_test.js | 2 +- src/node/test/health_test.js | 2 +- src/node/test/interop_sanity_test.js | 2 +- src/node/test/math_client_test.js | 2 +- src/node/test/server_test.js | 27 ++++++++++++++- src/node/test/surface_test.js | 16 ++++----- 10 files changed, 86 insertions(+), 41 deletions(-) diff --git a/src/node/ext/server.cc b/src/node/ext/server.cc index 8e39644846d..c32e3ae9182 100644 --- a/src/node/ext/server.cc +++ b/src/node/ext/server.cc @@ -139,8 +139,11 @@ void Server::Init(Handle exports) { NanSetPrototypeTemplate(tpl, "start", NanNew(Start)->GetFunction()); - NanSetPrototypeTemplate(tpl, "shutdown", - NanNew(Shutdown)->GetFunction()); + NanSetPrototypeTemplate(tpl, "tryShutdown", + NanNew(TryShutdown)->GetFunction()); + NanSetPrototypeTemplate( + tpl, "forceShutdown", + NanNew(ForceShutdown)->GetFunction()); NanAssignPersistent(fun_tpl, tpl); Handle ctr = tpl->GetFunction(); @@ -153,14 +156,13 @@ bool Server::HasInstance(Handle val) { } void Server::ShutdownServer() { - if (this->wrapped_server != NULL) { - grpc_server_shutdown_and_notify(this->wrapped_server, - this->shutdown_queue, - NULL); - grpc_completion_queue_pluck(this->shutdown_queue, NULL, - gpr_inf_future(GPR_CLOCK_REALTIME), NULL); - this->wrapped_server = NULL; - } + grpc_server_shutdown_and_notify(this->wrapped_server, + this->shutdown_queue, + NULL); + grpc_server_cancel_all_calls(this->wrapped_server); + grpc_completion_queue_pluck(this->shutdown_queue, NULL, + gpr_inf_future(GPR_CLOCK_REALTIME), NULL); + this->wrapped_server = NULL; } NAN_METHOD(Server::New) { @@ -222,9 +224,6 @@ NAN_METHOD(Server::RequestCall) { return NanThrowTypeError("requestCall can only be called on a Server"); } Server *server = ObjectWrap::Unwrap(args.This()); - if (server->wrapped_server == NULL) { - return NanThrowError("requestCall cannot be called on a shut down Server"); - } NewCallOp *op = new NewCallOp(); unique_ptr ops(new OpVec()); ops->push_back(unique_ptr(op)); @@ -256,10 +255,6 @@ NAN_METHOD(Server::AddHttp2Port) { "addHttp2Port's second argument must be ServerCredentials"); } Server *server = ObjectWrap::Unwrap(args.This()); - if (server->wrapped_server == NULL) { - return NanThrowError( - "addHttp2Port cannot be called on a shut down Server"); - } ServerCredentials *creds_object = ObjectWrap::Unwrap( args[1]->ToObject()); grpc_server_credentials *creds = creds_object->GetWrappedServerCredentials(); @@ -281,21 +276,30 @@ NAN_METHOD(Server::Start) { return NanThrowTypeError("start can only be called on a Server"); } Server *server = ObjectWrap::Unwrap(args.This()); - if (server->wrapped_server == NULL) { - return NanThrowError("start cannot be called on a shut down Server"); - } grpc_server_start(server->wrapped_server); NanReturnUndefined(); } -NAN_METHOD(ShutdownCallback) { +NAN_METHOD(Server::TryShutdown) { + NanScope(); + if (!HasInstance(args.This())) { + return NanThrowTypeError("tryShutdown can only be called on a Server"); + } + Server *server = ObjectWrap::Unwrap(args.This()); + unique_ptr ops(new OpVec()); + grpc_server_shutdown_and_notify( + server->wrapped_server, + CompletionQueueAsyncWorker::GetQueue(), + new struct tag(new NanCallback(args[0].As()), ops.release(), + shared_ptr(nullptr))); + CompletionQueueAsyncWorker::Next(); NanReturnUndefined(); } -NAN_METHOD(Server::Shutdown) { +NAN_METHOD(Server::ForceShutdown) { NanScope(); if (!HasInstance(args.This())) { - return NanThrowTypeError("shutdown can only be called on a Server"); + return NanThrowTypeError("forceShutdown can only be called on a Server"); } Server *server = ObjectWrap::Unwrap(args.This()); server->ShutdownServer(); diff --git a/src/node/ext/server.h b/src/node/ext/server.h index faab7e3418c..e7d5c3fb11a 100644 --- a/src/node/ext/server.h +++ b/src/node/ext/server.h @@ -67,7 +67,8 @@ class Server : public ::node::ObjectWrap { static NAN_METHOD(RequestCall); static NAN_METHOD(AddHttp2Port); static NAN_METHOD(Start); - static NAN_METHOD(Shutdown); + static NAN_METHOD(TryShutdown); + static NAN_METHOD(ForceShutdown); static NanCallback *constructor; static v8::Persistent fun_tpl; diff --git a/src/node/src/server.js b/src/node/src/server.js index 8b86173f082..f2520c3c970 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -613,11 +613,26 @@ function Server(options) { } server.requestCall(handleNewCall); }; + + /** + * Gracefully shuts down the server. The server will stop receiving new calls, + * and any pending calls will complete. The callback will be called when all + * pending calls have completed and the server is fully shut down. This method + * is idempotent with itself and forceShutdown. + * @param {function()} callback The shutdown complete callback + */ + this.tryShutdown = function(callback) { + server.tryShutdown(callback); + }; + /** - * Shuts down the server. + * Forcibly shuts down the server. The server will stop receiving new calls + * and cancel all pending calls. When it returns, the server has shut down. + * This method is idempotent with itself and tryShutdown, and it will trigger + * any outstanding tryShutdown callbacks. */ - this.shutdown = function() { - server.shutdown(); + this.forceShutdown = function() { + server.forceShutdown(); }; } diff --git a/src/node/test/call_test.js b/src/node/test/call_test.js index 8d0f20b0747..e7f071bcd53 100644 --- a/src/node/test/call_test.js +++ b/src/node/test/call_test.js @@ -61,7 +61,7 @@ describe('call', function() { channel = new grpc.Channel('localhost:' + port, insecureCreds); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); describe('constructor', function() { it('should reject anything less than 3 arguments', function() { diff --git a/src/node/test/end_to_end_test.js b/src/node/test/end_to_end_test.js index 7574d98b8af..4b8da3bfb17 100644 --- a/src/node/test/end_to_end_test.js +++ b/src/node/test/end_to_end_test.js @@ -70,7 +70,7 @@ describe('end-to-end', function() { channel = new grpc.Channel('localhost:' + port_num, insecureCreds); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('should start and end a request without error', function(complete) { var done = multiDone(complete, 2); diff --git a/src/node/test/health_test.js b/src/node/test/health_test.js index be4ef1d251b..22c58d3956b 100644 --- a/src/node/test/health_test.js +++ b/src/node/test/health_test.js @@ -61,7 +61,7 @@ describe('Health Checking', function() { grpc.Credentials.createInsecure()); }); after(function() { - healthServer.shutdown(); + healthServer.forceShutdown(); }); it('should say an enabled service is SERVING', function(done) { healthClient.check({service: ''}, function(err, response) { diff --git a/src/node/test/interop_sanity_test.js b/src/node/test/interop_sanity_test.js index 0a5eb29c0c1..2ca07c1d50d 100644 --- a/src/node/test/interop_sanity_test.js +++ b/src/node/test/interop_sanity_test.js @@ -51,7 +51,7 @@ describe('Interop tests', function() { done(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); // This depends on not using a binary stream it('should pass empty_unary', function(done) { diff --git a/src/node/test/math_client_test.js b/src/node/test/math_client_test.js index ef01870a4c1..80b0c5ff2a9 100644 --- a/src/node/test/math_client_test.js +++ b/src/node/test/math_client_test.js @@ -59,7 +59,7 @@ describe('Math client', function() { done(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('should handle a single request', function(done) { var arg = {dividend: 7, divisor: 4}; diff --git a/src/node/test/server_test.js b/src/node/test/server_test.js index 20c9a07ffa3..4670a62efa3 100644 --- a/src/node/test/server_test.js +++ b/src/node/test/server_test.js @@ -90,7 +90,7 @@ describe('server', function() { server.addHttp2Port('0.0.0.0:0', grpc.ServerCredentials.createInsecure()); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('should start without error', function() { assert.doesNotThrow(function() { @@ -98,4 +98,29 @@ describe('server', function() { }); }); }); + describe('shutdown', function() { + var server; + beforeEach(function() { + server = new grpc.Server(); + server.addHttp2Port('0.0.0.0:0', grpc.ServerCredentials.createInsecure()); + server.start(); + }); + afterEach(function() { + server.forceShutdown(); + }); + it('tryShutdown should shutdown successfully', function(done) { + server.tryShutdown(done); + }); + it.only('forceShutdown should shutdown successfully', function() { + server.forceShutdown(); + }); + it('tryShutdown should be idempotent', function(done) { + server.tryShutdown(done); + server.tryShutdown(function() {}); + }); + it('forceShutdown should be idempotent', function() { + server.forceShutdown(); + server.forceShutdown(); + }); + }); }); diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index 52515cc8e7c..d12ba0465e9 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -104,7 +104,7 @@ describe('Server.prototype.addProtoService', function() { server = new grpc.Server(); }); afterEach(function() { - server.shutdown(); + server.forceShutdown(); }); it('Should succeed with a single service', function() { assert.doesNotThrow(function() { @@ -148,7 +148,7 @@ describe('Client#$waitForReady', function() { client = new Client('localhost:' + port, grpc.Credentials.createInsecure()); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('should complete when called alone', function(done) { client.$waitForReady(Infinity, function(error) { @@ -203,7 +203,7 @@ describe('Echo service', function() { server.start(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('should echo the recieved message directly', function(done) { client.echo({value: 'test value', value2: 3}, function(error, response) { @@ -248,7 +248,7 @@ describe('Generic client and server', function() { grpc.Credentials.createInsecure()); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('Should respond with a capitalized string', function(done) { client.capitalize('abc', function(err, response) { @@ -296,7 +296,7 @@ describe('Echo metadata', function() { server.start(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('with unary call', function(done) { var call = client.unary({}, function(err, data) { @@ -419,7 +419,7 @@ describe('Other conditions', function() { server.start(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('channel.getTarget should be available', function() { assert.strictEqual(typeof client.channel.getTarget(), 'string'); @@ -681,7 +681,7 @@ describe('Other conditions', function() { }); afterEach(function() { console.log('Shutting down server'); - proxy.shutdown(); + proxy.forceShutdown(); }); describe('Cancellation', function() { it('With a unary call', function(done) { @@ -845,7 +845,7 @@ describe('Cancelling surface client', function() { server.start(); }); after(function() { - server.shutdown(); + server.forceShutdown(); }); it('Should correctly cancel a unary call', function(done) { var call = client.div({'divisor': 0, 'dividend': 0}, function(err, resp) { From 8a2ab3b249558f5bf4c6e4ef938c68b106750828 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 19 Aug 2015 10:34:59 -0700 Subject: [PATCH 02/12] Removed errant NULL setting --- src/node/ext/server.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node/ext/server.cc b/src/node/ext/server.cc index c32e3ae9182..57c43104907 100644 --- a/src/node/ext/server.cc +++ b/src/node/ext/server.cc @@ -120,7 +120,7 @@ Server::Server(grpc_server *server) : wrapped_server(server) { Server::~Server() { this->ShutdownServer(); grpc_completion_queue_shutdown(this->shutdown_queue); - grpc_server_destroy(wrapped_server); + grpc_server_destroy(this->wrapped_server); grpc_completion_queue_destroy(this->shutdown_queue); } @@ -162,7 +162,6 @@ void Server::ShutdownServer() { grpc_server_cancel_all_calls(this->wrapped_server); grpc_completion_queue_pluck(this->shutdown_queue, NULL, gpr_inf_future(GPR_CLOCK_REALTIME), NULL); - this->wrapped_server = NULL; } NAN_METHOD(Server::New) { From c5dac97bd3b1e87b228d0d130ce2cb457297fdd0 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 19 Aug 2015 11:49:53 -0700 Subject: [PATCH 03/12] Added a test, enabled other tests --- src/node/test/server_test.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/node/test/server_test.js b/src/node/test/server_test.js index 4670a62efa3..9574709f602 100644 --- a/src/node/test/server_test.js +++ b/src/node/test/server_test.js @@ -111,7 +111,7 @@ describe('server', function() { it('tryShutdown should shutdown successfully', function(done) { server.tryShutdown(done); }); - it.only('forceShutdown should shutdown successfully', function() { + it('forceShutdown should shutdown successfully', function() { server.forceShutdown(); }); it('tryShutdown should be idempotent', function(done) { @@ -122,5 +122,9 @@ describe('server', function() { server.forceShutdown(); server.forceShutdown(); }); + it('forceShutdown should trigger tryShutdown', function(done) { + server.tryShutdown(done); + server.forceShutdown(); + }); }); }); From 84e3cdeb970dae5840ab0453536a7a53428a8e65 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:27:05 -0700 Subject: [PATCH 04/12] Added new Metadata class to abstract over internal representation and normalize keys --- src/node/index.js | 15 +-- src/node/interop/interop_client.js | 8 +- src/node/src/client.js | 65 +++++++---- src/node/src/metadata.js | 167 ++++++++++++++++++++++++++++ src/node/src/server.js | 60 ++++++---- src/node/test/health_test.js | 2 +- src/node/test/metadata_test.js | 172 +++++++++++++++++++++++++++++ src/node/test/surface_test.js | 61 +++++----- 8 files changed, 460 insertions(+), 90 deletions(-) create mode 100644 src/node/src/metadata.js create mode 100644 src/node/test/metadata_test.js diff --git a/src/node/index.js b/src/node/index.js index 889b0ac0e92..51d3fa590cd 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -41,6 +41,8 @@ var client = require('./src/client.js'); var server = require('./src/server.js'); +var Metadata = require('./src/metadata.js'); + var grpc = require('bindings')('grpc'); /** @@ -107,18 +109,12 @@ exports.getGoogleAuthDelegate = function getGoogleAuthDelegate(credential) { * @param {function(Error, Object)} callback */ return function updateMetadata(authURI, metadata, callback) { - metadata = _.clone(metadata); - if (metadata.Authorization) { - metadata.Authorization = _.clone(metadata.Authorization); - } else { - metadata.Authorization = []; - } credential.getRequestMetadata(authURI, function(err, header) { if (err) { callback(err); return; } - metadata.Authorization.push(header.Authorization); + metadata.add('authorization', header.Authorization); callback(null, metadata); }); }; @@ -129,6 +125,11 @@ exports.getGoogleAuthDelegate = function getGoogleAuthDelegate(credential) { */ exports.Server = server.Server; +/** + * @see module:src/metadata + */ +exports.Metadata = Metadata; + /** * Status name to code number mapping */ diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index 612dcf01f65..8fb8d669206 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -321,13 +321,7 @@ function oauth2Test(expected_user, scope, per_rpc, client, done) { credential.getAccessToken(function(err, token) { assert.ifError(err); var updateMetadata = function(authURI, metadata, callback) { - metadata = _.clone(metadata); - if (metadata.Authorization) { - metadata.Authorization = _.clone(metadata.Authorization); - } else { - metadata.Authorization = []; - } - metadata.Authorization.push('Bearer ' + token); + metadata.Add('authorization', 'Bearer ' + token); callback(null, metadata); }; var makeTestCall = function(error, client_metadata) { diff --git a/src/node/src/client.js b/src/node/src/client.js index 48fe0dd3b77..6fafad251a2 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -42,7 +42,9 @@ var _ = require('lodash'); var grpc = require('bindings')('grpc.node'); -var common = require('./common.js'); +var common = require('./common'); + +var Metadata = require('./metadata'); var EventEmitter = require('events').EventEmitter; @@ -254,8 +256,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { * serialize * @param {function(?Error, value=)} callback The callback to for when the * response is received - * @param {array=} metadata Array of metadata key/value pairs to add to the - * call + * @param {Metadata=} metadata Metadata to add to the call * @param {Object=} options Options map * @return {EventEmitter} An event emitter for stream related events */ @@ -264,7 +265,9 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { var emitter = new EventEmitter(); var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } emitter.cancel = function cancel() { call.cancel(); @@ -281,7 +284,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { var client_batch = {}; var message = serialize(argument); message.grpcWriteFlags = options.flags; - client_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + client_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); client_batch[grpc.opType.SEND_MESSAGE] = message; client_batch[grpc.opType.SEND_CLOSE_FROM_CLIENT] = true; client_batch[grpc.opType.RECV_INITIAL_METADATA] = true; @@ -292,7 +296,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); callback(error); return; } else { @@ -302,7 +307,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { return; } } - emitter.emit('metadata', response.metadata); + emitter.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); callback(null, deserialize(response.read)); }); }); @@ -326,7 +332,7 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { * @this {Client} Client object. Must have a channel member. * @param {function(?Error, value=)} callback The callback to for when the * response is received - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Object=} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -335,7 +341,9 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientWritableStream(call, serialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -345,7 +353,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { return; } var metadata_batch = {}; - metadata_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + metadata_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); metadata_batch[grpc.opType.RECV_INITIAL_METADATA] = true; call.startBatch(metadata_batch, function(err, response) { if (err) { @@ -353,7 +362,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var client_batch = {}; client_batch[grpc.opType.RECV_MESSAGE] = true; @@ -363,7 +373,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); callback(error); return; } else { @@ -396,7 +407,7 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { * @this {SurfaceClient} Client object. Must have a channel member. * @param {*} argument The argument to the call. Should be serializable with * serialize - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Object} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -405,7 +416,9 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientReadableStream(call, deserialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -417,7 +430,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { var start_batch = {}; var message = serialize(argument); message.grpcWriteFlags = options.flags; - start_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + start_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); start_batch[grpc.opType.RECV_INITIAL_METADATA] = true; start_batch[grpc.opType.SEND_MESSAGE] = message; start_batch[grpc.opType.SEND_CLOSE_FROM_CLIENT] = true; @@ -427,7 +441,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; @@ -436,7 +451,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('error', error); return; } else { @@ -466,7 +482,7 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { /** * Make a bidirectional stream request with this method on the given channel. * @this {SurfaceClient} Client object. Must have a channel member. - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Options} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -475,7 +491,9 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientDuplexStream(call, serialize, deserialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -485,7 +503,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { return; } var start_batch = {}; - start_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + start_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); start_batch[grpc.opType.RECV_INITIAL_METADATA] = true; call.startBatch(start_batch, function(err, response) { if (err) { @@ -493,7 +512,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; @@ -502,7 +522,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('error', error); return; } else { diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js new file mode 100644 index 00000000000..ae7112f36e4 --- /dev/null +++ b/src/node/src/metadata.js @@ -0,0 +1,167 @@ +/* + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +/** + * Metadata module + * @module + */ + +'use strict'; + +var _ = require('lodash'); + +/** + * Class for storing metadata. Keys are normalized to lowercase ASCII. + * @constructor + */ +function Metadata() { + this._internal_repr = {}; +} + +function normalizeKey(key) { + return _.deburr(key).toLowerCase(); +} + +function validate(key, value) { + if (_.endsWith(key, '-bin')) { + if (!(value instanceof Buffer)) { + throw new Error('keys that end with \'-bin\' must have Buffer values'); + } + } else { + if (!_.isString(value)) { + throw new Error( + 'keys that don\'t end with \'-bin\' must have String values'); + } + } +} + +/** + * Sets the given value for the given key, replacing any other values associated + * with that key. Normalizes the key. + * @param {String} key The key to set + * @param {String|Buffer} value The value to set. Must be a buffer if and only + * if the normalized key ends with '-bin' + */ +Metadata.prototype.set = function(key, value) { + key = normalizeKey(key); + validate(key, value); + this._internal_repr[key] = [value]; +}; + +/** + * Adds the given value for the given key. Normalizes the key. + * @param {String} key The key to add to. + * @param {String|Buffer} value The value to add. Must be a buffer if and only + * if the normalized key ends with '-bin' + */ +Metadata.prototype.add = function(key, value) { + key = normalizeKey(key); + validate(key, value); + if (!this._internal_repr[key]) { + this._internal_repr[key] = []; + } + this._internal_repr[key].push(value); +}; + +/** + * Remove the given key and any associated values. + * @param {String} key The key to remove + */ +Metadata.prototype.remove = function(key) { + if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { + delete this._internal_repr[key]; + } +}; + +/** + * Gets a list of all values associated with the key. + * @param {String} key The key to get + * @return {Array.} The values associated with that key + */ +Metadata.prototype.get = function(key) { + if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { + return this._internal_repr[key]; + } else { + return []; + } +}; + +/** + * Get a map of each key to a single associated value. This reflects the most + * common way that people will want to see metadata. + * @return {Object.} A key/value mapping of the metadata + */ +Metadata.prototype.getMap = function() { + var result = {}; + _.forOwn(this._internal_repr, function(values, key) { + if(values.length > 0) { + result[key] = values[0]; + } + }); + return result; +}; + +/** + * Clone the metadata object. + * @return {Metadata} The new cloned object + */ +Metadata.prototype.clone = function() { + var copy = new Metadata(); + copy._internal_repr = _.cloneDeep(this._internal_repr); + return copy; +} + +/** + * Gets the metadata in the format used by interal code. Intended for internal + * use only. API stability is not guaranteed. + * @private + * @return {Object.>} The metadata + */ +Metadata.prototype._getCoreRepresentation = function() { + return this._internal_repr; +}; + +/** + * Creates a Metadata object from a metadata map in the internal format. + * Intended for internal use only. API stability is not guaranteed. + * @private + * @param {Object.>} The metadata + * @return {Metadata} The new Metadata object + */ +Metadata._fromCoreRepresentation = function(metadata) { + var newMetadata = new Metadata(); + newMetadata._internal_repr = _.cloneDeep(metadata); + return newMetadata; +}; + +module.exports = Metadata; diff --git a/src/node/src/server.js b/src/node/src/server.js index 5037abae434..5d76b31f157 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -44,6 +44,8 @@ var grpc = require('bindings')('grpc.node'); var common = require('./common'); +var Metadata = require('./metadata'); + var stream = require('stream'); var Readable = stream.Readable; @@ -60,10 +62,10 @@ var EventEmitter = require('events').EventEmitter; * @param {Object} error The error object */ function handleError(call, error) { + var statusMetadata = new Metadata(); var status = { code: grpc.status.UNKNOWN, - details: 'Unknown Error', - metadata: {} + details: 'Unknown Error' }; if (error.hasOwnProperty('message')) { status.details = error.message; @@ -75,11 +77,13 @@ function handleError(call, error) { } } if (error.hasOwnProperty('metadata')) { - status.metadata = error.metadata; + statusMetadata = error.metadata; } + status.metadata = statusMetadata._getCoreRepresentation(); var error_batch = {}; if (!call.metadataSent) { - error_batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + error_batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); } error_batch[grpc.opType.SEND_STATUS_FROM_SERVER] = status; call.startBatch(error_batch, function(){}); @@ -114,22 +118,24 @@ function waitForCancel(call, emitter) { * @param {*} value The value to respond with * @param {function(*):Buffer=} serialize Serialization function for the * response - * @param {Object=} metadata Optional trailing metadata to send with status + * @param {Metadata=} metadata Optional trailing metadata to send with status * @param {number=} flags Flags for modifying how the message is sent. * Defaults to 0. */ function sendUnaryResponse(call, value, serialize, metadata, flags) { var end_batch = {}; + var statusMetadata = new Metadata(); var status = { code: grpc.status.OK, - details: 'OK', - metadata: {} + details: 'OK' }; if (metadata) { - status.metadata = metadata; + statusMetadata = metadata; } + status.metadata = statusMetadata._getCoreRepresentation(); if (!call.metadataSent) { - end_batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + end_batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); call.metadataSent = true; } var message = serialize(value); @@ -151,15 +157,17 @@ function setUpWritable(stream, serialize) { stream.status = { code : grpc.status.OK, details : 'OK', - metadata : {} + metadata : new Metadata() }; stream.serialize = common.wrapIgnoreNull(serialize); function sendStatus() { var batch = {}; if (!stream.call.metadataSent) { stream.call.metadataSent = true; - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); } + stream.status.metadata = stream.status.metadata._getCoreRepresentation(); batch[grpc.opType.SEND_STATUS_FROM_SERVER] = stream.status; stream.call.startBatch(batch, function(){}); } @@ -203,7 +211,7 @@ function setUpWritable(stream, serialize) { /** * Override of Writable#end method that allows for sending metadata with a * success status. - * @param {Object=} metadata Metadata to send with the status + * @param {Metadata=} metadata Metadata to send with the status */ stream.end = function(metadata) { if (metadata) { @@ -266,7 +274,8 @@ function _write(chunk, encoding, callback) { /* jshint validthis: true */ var batch = {}; if (!this.call.metadataSent) { - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); this.call.metadataSent = true; } var message = this.serialize(chunk); @@ -289,15 +298,15 @@ ServerWritableStream.prototype._write = _write; /** * Send the initial metadata for a writable stream. - * @param {Object>} responseMetadata Metadata - * to send + * @param {Metadata} responseMetadata Metadata to send */ function sendMetadata(responseMetadata) { /* jshint validthis: true */ if (!this.call.metadataSent) { this.call.metadataSent = true; var batch = []; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); this.call.startBatch(batch, function(err) { if (err) { this.emit('error', err); @@ -422,7 +431,7 @@ ServerDuplexStream.prototype.getPeer = getPeer; * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleUnary(call, handler, metadata) { var emitter = new EventEmitter(); @@ -430,7 +439,8 @@ function handleUnary(call, handler, metadata) { if (!call.metadataSent) { call.metadataSent = true; var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); call.startBatch(batch, function() {}); } }; @@ -478,7 +488,7 @@ function handleUnary(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleServerStreaming(call, handler, metadata) { var stream = new ServerWritableStream(call, handler.serialize); @@ -507,7 +517,7 @@ function handleServerStreaming(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleClientStreaming(call, handler, metadata) { var stream = new ServerReadableStream(call, handler.deserialize); @@ -515,7 +525,8 @@ function handleClientStreaming(call, handler, metadata) { if (!call.metadataSent) { call.metadataSent = true; var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); call.startBatch(batch, function() {}); } }; @@ -542,7 +553,7 @@ function handleClientStreaming(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleBidiStreaming(call, handler, metadata) { var stream = new ServerDuplexStream(call, handler.serialize, @@ -599,7 +610,7 @@ function Server(options) { var details = event.new_call; var call = details.call; var method = details.method; - var metadata = details.metadata; + var metadata = Metadata._fromCoreRepresentation(details.metadata); if (method === null) { return; } @@ -609,7 +620,8 @@ function Server(options) { handler = handlers[method]; } else { var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); batch[grpc.opType.SEND_STATUS_FROM_SERVER] = { code: grpc.status.UNIMPLEMENTED, details: 'This method is not available on this server.', diff --git a/src/node/test/health_test.js b/src/node/test/health_test.js index be4ef1d251b..5e6a0ba4670 100644 --- a/src/node/test/health_test.js +++ b/src/node/test/health_test.js @@ -39,7 +39,7 @@ var health = require('../health_check/health.js'); var grpc = require('../'); -describe('Health Checking', function() { +describe.only('Health Checking', function() { var statusMap = { '': { '': 'SERVING', diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js new file mode 100644 index 00000000000..f1859b674d8 --- /dev/null +++ b/src/node/test/metadata_test.js @@ -0,0 +1,172 @@ +/* + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +'use strict'; + +var Metadata = require('../src/metadata.js'); + +var assert = require('assert'); + +describe('Metadata', function() { + var metadata; + beforeEach(function() { + metadata = new Metadata(); + }); + describe('#set', function() { + it('Only accepts string values for non "-bin" keys', function() { + assert.throws(function() { + metadata.set('key', new Buffer('value')); + }); + assert.doesNotThrow(function() { + metadata.set('key', 'value'); + }); + }); + it('Only accepts Buffer values for "-bin" keys', function() { + assert.throws(function() { + metadata.set('key-bin', 'value'); + }); + assert.doesNotThrow(function() { + metadata.set('key-bin', new Buffer('value')); + }); + }); + it('Saves values that can be retrieved', function() { + metadata.set('key', 'value'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + it('Overwrites previous values', function() { + metadata.set('key', 'value1'); + metadata.set('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value2']); + }); + it('Normalizes keys', function() { + metadata.set('Key', 'value1'); + assert.deepEqual(metadata.get('key'), ['value1']); + metadata.set('KEY', 'value2'); + assert.deepEqual(metadata.get('key'), ['value2']); + }); + }); + describe('#add', function() { + it('Only accepts string values for non "-bin" keys', function() { + assert.throws(function() { + metadata.add('key', new Buffer('value')); + }); + assert.doesNotThrow(function() { + metadata.add('key', 'value'); + }); + }); + it('Only accepts Buffer values for "-bin" keys', function() { + assert.throws(function() { + metadata.add('key-bin', 'value'); + }); + assert.doesNotThrow(function() { + metadata.add('key-bin', new Buffer('value')); + }); + }); + it('Saves values that can be retrieved', function() { + metadata.add('key', 'value'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + it('Combines with previous values', function() { + metadata.add('key', 'value1'); + metadata.add('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + it('Normalizes keys', function() { + metadata.add('Key', 'value1'); + assert.deepEqual(metadata.get('key'), ['value1']); + metadata.add('KEY', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + }); + describe('#remove', function() { + it('clears values from a key', function() { + metadata.add('key', 'value'); + metadata.remove('key'); + assert.deepEqual(metadata.get('key'), []); + }); + it('does not normalize keys', function() { + metadata.add('key', 'value'); + metadata.remove('KEY'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + }); + describe('#get', function() { + beforeEach(function() { + metadata.add('key', 'value1'); + metadata.add('key', 'value2'); + metadata.add('key-bin', new Buffer('value')); + }); + it('gets all values associated with a key', function() { + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + it('does not normalize keys', function() { + assert.deepEqual(metadata.get('KEY'), []); + }); + it('returns an empty list for non-existent keys', function() { + assert.deepEqual(metadata.get('non-existent-key'), []); + }); + it('returns Buffers for "-bin" keys', function() { + assert(metadata.get('key-bin')[0] instanceof Buffer); + }); + }); + describe('#getMap', function() { + it('gets a map of keys to values', function() { + metadata.add('key1', 'value1'); + metadata.add('Key2', 'value2'); + metadata.add('KEY3', 'value3'); + assert.deepEqual(metadata.getMap(), + {key1: 'value1', + key2: 'value2', + key3: 'value3'}); + }); + }); + describe('#clone', function() { + it('retains values from the original', function() { + metadata.add('key', 'value'); + var copy = metadata.clone(); + assert.deepEqual(copy.get('key'), ['value']); + }); + it('Does not see newly added values', function() { + metadata.add('key', 'value1'); + var copy = metadata.clone(); + metadata.add('key', 'value2'); + assert.deepEqual(copy.get('key'), ['value1']); + }); + it('Does not add new values to the original', function() { + metadata.add('key', 'value1'); + var copy = metadata.clone(); + copy.add('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1']); + }); + }); +}); diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index ec7ed87728d..c3caa4d5a38 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -262,6 +262,7 @@ describe('Generic client and server', function() { describe('Echo metadata', function() { var client; var server; + var metadata; before(function() { var test_proto = ProtoBuf.loadProtoFile(__dirname + '/test_service.proto'); var test_service = test_proto.lookup('TestService'); @@ -294,6 +295,8 @@ describe('Echo metadata', function() { var Client = surface_client.makeProtobufClientConstructor(test_service); client = new Client('localhost:' + port, grpc.Credentials.createInsecure()); server.start(); + metadata = new grpc.Metadata(); + metadata.set('key', 'value'); }); after(function() { server.shutdown(); @@ -301,35 +304,35 @@ describe('Echo metadata', function() { it('with unary call', function(done) { var call = client.unary({}, function(err, data) { assert.ifError(err); - }, {key: ['value']}); + }, metadata); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); }); it('with client stream call', function(done) { var call = client.clientStream(function(err, data) { assert.ifError(err); - }, {key: ['value']}); + }, metadata); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); call.end(); }); it('with server stream call', function(done) { - var call = client.serverStream({}, {key: ['value']}); + var call = client.serverStream({}, metadata); call.on('data', function() {}); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); }); it('with bidi stream call', function(done) { - var call = client.bidiStream({key: ['value']}); + var call = client.bidiStream(metadata); call.on('data', function() {}); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); call.end(); @@ -337,9 +340,10 @@ describe('Echo metadata', function() { it('shows the correct user-agent string', function(done) { var version = require('../package.json').version; var call = client.unary({}, function(err, data) { assert.ifError(err); }, - {key: ['value']}); + metadata); call.on('metadata', function(metadata) { - assert(_.startsWith(metadata['user-agent'], 'grpc-node/' + version)); + assert(_.startsWith(metadata.get('user-agent')[0], + 'grpc-node/' + version)); done(); }); }); @@ -354,13 +358,14 @@ describe('Other conditions', function() { var test_proto = ProtoBuf.loadProtoFile(__dirname + '/test_service.proto'); test_service = test_proto.lookup('TestService'); server = new grpc.Server(); + var trailer_metadata = new grpc.Metadata(); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; if (req.error) { - cb(new Error('Requested error'), null, {trailer_present: ['yes']}); + cb(new Error('Requested error'), null, trailer_metadata); } else { - cb(null, {count: 1}, {trailer_present: ['yes']}); + cb(null, {count: 1}, trailer_metadata); } }, clientStream: function(stream, cb){ @@ -369,14 +374,14 @@ describe('Other conditions', function() { stream.on('data', function(data) { if (data.error) { errored = true; - cb(new Error('Requested error'), null, {trailer_present: ['yes']}); + cb(new Error('Requested error'), null, trailer_metadata); } else { count += 1; } }); stream.on('end', function() { if (!errored) { - cb(null, {count: count}, {trailer_present: ['yes']}); + cb(null, {count: count}, trailer_metadata); } }); }, @@ -384,13 +389,13 @@ describe('Other conditions', function() { var req = stream.request; if (req.error) { var err = new Error('Requested error'); - err.metadata = {trailer_present: ['yes']}; + err.metadata = trailer_metadata; stream.emit('error', err); } else { for (var i = 0; i < 5; i++) { stream.write({count: i}); } - stream.end({trailer_present: ['yes']}); + stream.end(trailer_metadata); } }, bidiStream: function(stream) { @@ -398,10 +403,8 @@ describe('Other conditions', function() { stream.on('data', function(data) { if (data.error) { var err = new Error('Requested error'); - err.metadata = { - trailer_present: ['yes'], - count: ['' + count] - }; + err.metadata = trailer_metadata.clone(); + err.metadata.add('count', '' + count); stream.emit('error', err); } else { stream.write({count: count}); @@ -409,7 +412,7 @@ describe('Other conditions', function() { } }); stream.on('end', function() { - stream.end({trailer_present: ['yes']}); + stream.end(trailer_metadata); }); } }); @@ -510,7 +513,7 @@ describe('Other conditions', function() { assert.ifError(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -519,7 +522,7 @@ describe('Other conditions', function() { assert(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -531,7 +534,7 @@ describe('Other conditions', function() { call.write({error: false}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -543,7 +546,7 @@ describe('Other conditions', function() { call.write({error: true}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -552,7 +555,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -560,7 +563,7 @@ describe('Other conditions', function() { var call = client.serverStream({error: true}); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.trailer_present, ['yes']); + assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -572,7 +575,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -583,7 +586,7 @@ describe('Other conditions', function() { call.end(); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.trailer_present, ['yes']); + assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); done(); }); }); From e796e1fd7c6a01825745a308309ad8621202e53a Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:31:56 -0700 Subject: [PATCH 05/12] Re-enabled tests --- src/node/test/health_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/test/health_test.js b/src/node/test/health_test.js index 5e6a0ba4670..be4ef1d251b 100644 --- a/src/node/test/health_test.js +++ b/src/node/test/health_test.js @@ -39,7 +39,7 @@ var health = require('../health_check/health.js'); var grpc = require('../'); -describe.only('Health Checking', function() { +describe('Health Checking', function() { var statusMap = { '': { '': 'SERVING', From f441b3fdf2fb85076380502dd50025246e76130e Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:44:52 -0700 Subject: [PATCH 06/12] Fixed test and lint errors --- src/node/src/client.js | 20 ++++++++++++-------- src/node/src/metadata.js | 6 ++++-- src/node/src/server.js | 7 +++++-- src/node/test/surface_test.js | 1 + 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/node/src/client.js b/src/node/src/client.js index b90af42054a..e1bed3512e9 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -294,12 +294,13 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { client_batch[grpc.opType.RECV_MESSAGE] = true; client_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(client_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); emitter.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; callback(error); return; } else { @@ -371,12 +372,13 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { client_batch[grpc.opType.RECV_MESSAGE] = true; client_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(client_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; callback(error); return; } else { @@ -451,12 +453,13 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(status_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; stream.emit('error', error); return; } else { @@ -522,12 +525,13 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(status_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; stream.emit('error', error); return; } else { diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index ae7112f36e4..39514b25476 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -139,7 +139,7 @@ Metadata.prototype.clone = function() { var copy = new Metadata(); copy._internal_repr = _.cloneDeep(this._internal_repr); return copy; -} +}; /** * Gets the metadata in the format used by interal code. Intended for internal @@ -160,7 +160,9 @@ Metadata.prototype._getCoreRepresentation = function() { */ Metadata._fromCoreRepresentation = function(metadata) { var newMetadata = new Metadata(); - newMetadata._internal_repr = _.cloneDeep(metadata); + if (metadata) { + newMetadata._internal_repr = _.cloneDeep(metadata); + } return newMetadata; }; diff --git a/src/node/src/server.js b/src/node/src/server.js index 7ef28428b45..b6f162adf85 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -167,7 +167,10 @@ function setUpWritable(stream, serialize) { batch[grpc.opType.SEND_INITIAL_METADATA] = (new Metadata())._getCoreRepresentation(); } - stream.status.metadata = stream.status.metadata._getCoreRepresentation(); + + if (stream.status.metadata) { + stream.status.metadata = stream.status.metadata._getCoreRepresentation(); + } batch[grpc.opType.SEND_STATUS_FROM_SERVER] = stream.status; stream.call.startBatch(batch, function(){}); } @@ -181,7 +184,7 @@ function setUpWritable(stream, serialize) { function setStatus(err) { var code = grpc.status.UNKNOWN; var details = 'Unknown Error'; - var metadata = {}; + var metadata = new Metadata(); if (err.hasOwnProperty('message')) { details = err.message; } diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index f000983a4aa..c7e63e98141 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -359,6 +359,7 @@ describe('Other conditions', function() { test_service = test_proto.lookup('TestService'); server = new grpc.Server(); var trailer_metadata = new grpc.Metadata(); + trailer_metadata.add('trailer_present', 'yes'); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; From cc248a27f2e87c0cbc649c78f6db323982a6c7ba Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 14:47:15 -0700 Subject: [PATCH 07/12] Added string value validation, modified key normalization and validation --- src/node/src/metadata.js | 10 +++++++++- src/node/test/metadata_test.js | 21 +++++++++++++++++++++ src/node/test/surface_test.js | 18 +++++++++--------- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 39514b25476..8e0884acea6 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,10 +49,14 @@ function Metadata() { } function normalizeKey(key) { - return _.deburr(key).toLowerCase(); + return key.toLowerCase(); } function validate(key, value) { + if (!(/^[a-z\d-]+$/.test(key))) { + throw new Error('Metadata keys must be nonempty strings containing only ' + + 'alphanumeric characters and hyphens'); + } if (_.endsWith(key, '-bin')) { if (!(value instanceof Buffer)) { throw new Error('keys that end with \'-bin\' must have Buffer values'); @@ -62,6 +66,10 @@ function validate(key, value) { throw new Error( 'keys that don\'t end with \'-bin\' must have String values'); } + if (!(/^[\x20-\x7E]*$/.test(value))) { + throw new Error('Metadata string values can only contain printable ' + + 'ASCII characters and space'); + } } } diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js index f1859b674d8..227fa9c9949 100644 --- a/src/node/test/metadata_test.js +++ b/src/node/test/metadata_test.js @@ -59,6 +59,19 @@ describe('Metadata', function() { metadata.set('key-bin', new Buffer('value')); }); }); + it('Rejects invalid keys', function() { + assert.throws(function() { + metadata.set('key$', 'value'); + }); + assert.throws(function() { + metadata.set('', 'value'); + }); + }); + it('Rejects values with non-ASCII characters', function() { + assert.throws(function() { + metadata.set('key', 'résumé'); + }); + }); it('Saves values that can be retrieved', function() { metadata.set('key', 'value'); assert.deepEqual(metadata.get('key'), ['value']); @@ -92,6 +105,14 @@ describe('Metadata', function() { metadata.add('key-bin', new Buffer('value')); }); }); + it('Rejects invalid keys', function() { + assert.throws(function() { + metadata.add('key$', 'value'); + }); + assert.throws(function() { + metadata.add('', 'value'); + }); + }); it('Saves values that can be retrieved', function() { metadata.add('key', 'value'); assert.deepEqual(metadata.get('key'), ['value']); diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index c7e63e98141..7c2a8d72583 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -359,7 +359,7 @@ describe('Other conditions', function() { test_service = test_proto.lookup('TestService'); server = new grpc.Server(); var trailer_metadata = new grpc.Metadata(); - trailer_metadata.add('trailer_present', 'yes'); + trailer_metadata.add('trailer-present', 'yes'); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; @@ -514,7 +514,7 @@ describe('Other conditions', function() { assert.ifError(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -523,7 +523,7 @@ describe('Other conditions', function() { assert(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -535,7 +535,7 @@ describe('Other conditions', function() { call.write({error: false}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -547,7 +547,7 @@ describe('Other conditions', function() { call.write({error: true}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -556,7 +556,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -564,7 +564,7 @@ describe('Other conditions', function() { var call = client.serverStream({error: true}); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(error.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -576,7 +576,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -587,7 +587,7 @@ describe('Other conditions', function() { call.end(); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(error.metadata.get('trailer-present'), ['yes']); done(); }); }); From 6b8a3a74f27a81940023ba3a8c822078d51621ee Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 14:51:59 -0700 Subject: [PATCH 08/12] Normalize keys when getting and removing metadata items --- src/node/src/metadata.js | 6 ++++-- src/node/test/metadata_test.js | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 8e0884acea6..a7b1ee3810d 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -102,21 +102,23 @@ Metadata.prototype.add = function(key, value) { }; /** - * Remove the given key and any associated values. + * Remove the given key and any associated values. Normalizes the key. * @param {String} key The key to remove */ Metadata.prototype.remove = function(key) { + key = normalizeKey(key); if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { delete this._internal_repr[key]; } }; /** - * Gets a list of all values associated with the key. + * Gets a list of all values associated with the key. Normalizes the key. * @param {String} key The key to get * @return {Array.} The values associated with that key */ Metadata.prototype.get = function(key) { + key = normalizeKey(key); if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { return this._internal_repr[key]; } else { diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js index 227fa9c9949..86383f1badc 100644 --- a/src/node/test/metadata_test.js +++ b/src/node/test/metadata_test.js @@ -135,10 +135,10 @@ describe('Metadata', function() { metadata.remove('key'); assert.deepEqual(metadata.get('key'), []); }); - it('does not normalize keys', function() { + it('Normalizes keys', function() { metadata.add('key', 'value'); metadata.remove('KEY'); - assert.deepEqual(metadata.get('key'), ['value']); + assert.deepEqual(metadata.get('key'), []); }); }); describe('#get', function() { @@ -150,8 +150,8 @@ describe('Metadata', function() { it('gets all values associated with a key', function() { assert.deepEqual(metadata.get('key'), ['value1', 'value2']); }); - it('does not normalize keys', function() { - assert.deepEqual(metadata.get('KEY'), []); + it('Normalizes keys', function() { + assert.deepEqual(metadata.get('KEY'), ['value1', 'value2']); }); it('returns an empty list for non-existent keys', function() { assert.deepEqual(metadata.get('non-existent-key'), []); From 01a772028041577099e406019b731b3540a6fb2f Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 15:40:03 -0700 Subject: [PATCH 09/12] Moved key character check to before key transformation --- src/node/src/metadata.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index a7b1ee3810d..d4a8b2669ff 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,14 +49,14 @@ function Metadata() { } function normalizeKey(key) { + if (!(/^[A-Za-z\d-]+$/.test(key))) { + throw new Error('Metadata keys must be nonempty strings containing only ' + + 'alphanumeric characters and hyphens'); + } return key.toLowerCase(); } function validate(key, value) { - if (!(/^[a-z\d-]+$/.test(key))) { - throw new Error('Metadata keys must be nonempty strings containing only ' + - 'alphanumeric characters and hyphens'); - } if (_.endsWith(key, '-bin')) { if (!(value instanceof Buffer)) { throw new Error('keys that end with \'-bin\' must have Buffer values'); From 5df6ebd0c585756467c128dc20b60be515163f5e Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 15:52:57 -0700 Subject: [PATCH 10/12] Replaced toLowerCase with local-insensitive downcasing function --- src/node/src/metadata.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index d4a8b2669ff..77ababb65df 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -48,12 +48,19 @@ function Metadata() { this._internal_repr = {}; } +function downcaseString(str) { + var capitals = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; + var lowercase = 'abcdefghijklmnopqrstuvwxyz'; + var charMap = _.zipObject(capitals, lowercase); + return str.replace(/[A-Z]/g, _.curry(_.get)(charMap)); +} + function normalizeKey(key) { if (!(/^[A-Za-z\d-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); } - return key.toLowerCase(); + return downcaseString(key); } function validate(key, value) { From 61bb21ab2c18fb7db403ae4c762c719d42c11680 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 21 Aug 2015 09:15:19 -0700 Subject: [PATCH 11/12] Reversed toLowerCase removal --- src/node/src/metadata.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 77ababb65df..d4a8b2669ff 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -48,19 +48,12 @@ function Metadata() { this._internal_repr = {}; } -function downcaseString(str) { - var capitals = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; - var lowercase = 'abcdefghijklmnopqrstuvwxyz'; - var charMap = _.zipObject(capitals, lowercase); - return str.replace(/[A-Z]/g, _.curry(_.get)(charMap)); -} - function normalizeKey(key) { if (!(/^[A-Za-z\d-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); } - return downcaseString(key); + return key.toLowerCase(); } function validate(key, value) { From dde19d835e024aa39a778990b5efffef58b4fb8d Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 21 Aug 2015 09:24:33 -0700 Subject: [PATCH 12/12] Allowed underscore in metadata keys --- src/node/src/metadata.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index d4a8b2669ff..65fd91f3672 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,7 +49,7 @@ function Metadata() { } function normalizeKey(key) { - if (!(/^[A-Za-z\d-]+$/.test(key))) { + if (!(/^[A-Za-z\d_-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); }