From a33965a0c003da42854dbcea162d7554261ad93b Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 29 Jul 2015 10:09:36 -0700 Subject: [PATCH 1/4] Ensure that client generated methods don't conflict with other properties --- src/client.js | 27 +++++++++++++++------------ test/surface_test.js | 20 +++++++++++++++++++- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/client.js b/src/client.js index f843669b..405e2be6 100644 --- a/src/client.js +++ b/src/client.js @@ -236,7 +236,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { deadline = Infinity; } var emitter = new EventEmitter(); - var call = new grpc.Call(this.channel, method, deadline); + var call = new grpc.Call(this.$channel, method, deadline); if (metadata === null || metadata === undefined) { metadata = {}; } @@ -246,7 +246,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { emitter.getPeer = function getPeer() { return call.getPeer(); }; - this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -309,12 +309,12 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { if (deadline === undefined) { deadline = Infinity; } - var call = new grpc.Call(this.channel, method, deadline); + var call = new grpc.Call(this.$channel, method, deadline); if (metadata === null || metadata === undefined) { metadata = {}; } var stream = new ClientWritableStream(call, serialize); - this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -383,12 +383,12 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { if (deadline === undefined) { deadline = Infinity; } - var call = new grpc.Call(this.channel, method, deadline); + var call = new grpc.Call(this.$channel, method, deadline); if (metadata === null || metadata === undefined) { metadata = {}; } var stream = new ClientReadableStream(call, deserialize); - this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -455,12 +455,12 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { if (deadline === undefined) { deadline = Infinity; } - var call = new grpc.Call(this.channel, method, deadline); + var call = new grpc.Call(this.$channel, method, deadline); if (metadata === null || metadata === undefined) { metadata = {}; } var stream = new ClientDuplexStream(call, serialize, deserialize); - this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -545,14 +545,17 @@ exports.makeClientConstructor = function(methods, serviceName) { options = {}; } options['grpc.primary_user_agent'] = 'grpc-node/' + version; - this.channel = new grpc.Channel(address, options); - this.server_address = address.replace(/\/$/, ''); - this.auth_uri = this.server_address + '/' + serviceName; - this.updateMetadata = updateMetadata; + this.$channel = new grpc.Channel(address, options); + this.$server_address = address.replace(/\/$/, ''); + this.$auth_uri = this.$server_address + '/' + serviceName; + this.$updateMetadata = updateMetadata; } _.each(methods, function(attrs, name) { var method_type; + if (_.startsWith(name, '$')) { + throw new Error('Method names cannot start with $'); + } if (attrs.requestStream) { if (attrs.responseStream) { method_type = 'bidi'; diff --git a/test/surface_test.js b/test/surface_test.js index 98f9b15b..1afdb330 100644 --- a/test/surface_test.js +++ b/test/surface_test.js @@ -110,6 +110,24 @@ describe('Server.prototype.addProtoService', function() { }); }); }); +describe('Client constructor building', function() { + var illegal_service_attrs = { + $method : { + path: '/illegal/$method', + requestStream: false, + responseStream: false, + requestSerialize: _.identity, + requestDeserialize: _.identity, + responseSerialize: _.identity, + responseDeserialize: _.identity + } + }; + it('Should reject method names starting with $', function() { + assert.throws(function() { + grpc.makeGenericClientConstructor(illegal_service_attrs); + }, /\$/); + }); +}); describe('Echo service', function() { var server; var client; @@ -344,7 +362,7 @@ describe('Other conditions', function() { server.shutdown(); }); it('channel.getTarget should be available', function() { - assert.strictEqual(typeof client.channel.getTarget(), 'string'); + assert.strictEqual(typeof client.$channel.getTarget(), 'string'); }); describe('Server recieving bad input', function() { var misbehavingClient; From 8aa5309e37459e9c6ed516ec338019af24d861a4 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 29 Jul 2015 15:25:57 -0700 Subject: [PATCH 2/4] Missed one --- interop/interop_client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/interop/interop_client.js b/interop/interop_client.js index e810e68e..953cbb16 100644 --- a/interop/interop_client.js +++ b/interop/interop_client.js @@ -292,7 +292,7 @@ function authTest(expected_user, scope, client, done) { if (credential.createScopedRequired() && scope) { credential = credential.createScoped(scope); } - client.updateMetadata = grpc.getGoogleAuthDelegate(credential); + client.$updateMetadata = grpc.getGoogleAuthDelegate(credential); var arg = { response_type: 'COMPRESSABLE', response_size: 314159, @@ -355,7 +355,7 @@ function oauth2Test(expected_user, scope, per_rpc, client, done) { if (per_rpc) { updateMetadata('', {}, makeTestCall); } else { - client.updateMetadata = updateMetadata; + client.$updateMetadata = updateMetadata; makeTestCall(null, {}); } From 3954abe9779e732f3700a16a476cc0576dc4eb11 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 21 Aug 2015 14:01:47 -0700 Subject: [PATCH 3/4] Changed prefixed Client properties to distinguish private and public properties --- interop/interop_client.js | 4 ++-- src/client.js | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/interop/interop_client.js b/interop/interop_client.js index 9100edf2..2f8eaea4 100644 --- a/interop/interop_client.js +++ b/interop/interop_client.js @@ -285,7 +285,7 @@ function authTest(expected_user, scope, client, done) { if (credential.createScopedRequired() && scope) { credential = credential.createScoped(scope); } - client.$updateMetadata = grpc.getGoogleAuthDelegate(credential); + client.$_updateMetadata = grpc.getGoogleAuthDelegate(credential); var arg = { response_type: 'COMPRESSABLE', response_size: 314159, @@ -344,7 +344,7 @@ function oauth2Test(expected_user, scope, per_rpc, client, done) { if (per_rpc) { updateMetadata('', {}, makeTestCall); } else { - client.$updateMetadata = updateMetadata; + client.$_updateMetadata = updateMetadata; makeTestCall(null, {}); } }); diff --git a/src/client.js b/src/client.js index f288104e..ddd28e37 100644 --- a/src/client.js +++ b/src/client.js @@ -32,7 +32,7 @@ */ /** - * Server module + * Client module * @module */ @@ -272,7 +272,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { emitter.getPeer = function getPeer() { return call.getPeer(); }; - this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { + this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -340,7 +340,7 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { metadata = {}; } var stream = new ClientWritableStream(call, serialize); - this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { + this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -410,7 +410,7 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { metadata = {}; } var stream = new ClientReadableStream(call, deserialize); - this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { + this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -482,7 +482,7 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { metadata = {}; } var stream = new ClientDuplexStream(call, serialize, deserialize); - this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { + this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -572,9 +572,9 @@ exports.makeClientConstructor = function(methods, serviceName) { this.$channel = new grpc.Channel(address, credentials, options); // Remove the optional DNS scheme, trailing port, and trailing backslash address = address.replace(/^(dns:\/{3})?([^:\/]+)(:\d+)?\/?$/, '$2'); - this.$server_address = address; - this.$auth_uri = 'https://' + this.server_address + '/' + serviceName; - this.$updateMetadata = updateMetadata; + this.$_server_address = address; + this.$_auth_uri = 'https://' + this.server_address + '/' + serviceName; + this.$_updateMetadata = updateMetadata; } /** From d76b6a0c4f355bd24541f65d6090790ea93f8709 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 28 Aug 2015 14:57:04 -0700 Subject: [PATCH 4/4] Switched to using static functions for accessing Client properties --- index.js | 10 +++++ interop/interop_client.js | 4 +- src/client.js | 83 ++++++++++++++++++++++----------------- test/surface_test.js | 17 ++++---- 4 files changed, 68 insertions(+), 46 deletions(-) diff --git a/index.js b/index.js index 51d3fa59..02b73f66 100644 --- a/index.js +++ b/index.js @@ -164,3 +164,13 @@ exports.ServerCredentials = grpc.ServerCredentials; * @see module:src/client.makeClientConstructor */ exports.makeGenericClientConstructor = client.makeClientConstructor; + +/** + * @see module:src/client.getClientChannel + */ +exports.getClientChannel = client.getClientChannel; + +/** + * @see module:src/client.waitForClientReady + */ +exports.waitForClientReady = client.waitForClientReady; diff --git a/interop/interop_client.js b/interop/interop_client.js index da5e0f4a..6a8d2633 100644 --- a/interop/interop_client.js +++ b/interop/interop_client.js @@ -285,7 +285,7 @@ function authTest(expected_user, scope, client, done) { if (credential.createScopedRequired() && scope) { credential = credential.createScoped(scope); } - client.$_updateMetadata = grpc.getGoogleAuthDelegate(credential); + client.$updateMetadata = grpc.getGoogleAuthDelegate(credential); var arg = { response_type: 'COMPRESSABLE', response_size: 314159, @@ -338,7 +338,7 @@ function oauth2Test(expected_user, scope, per_rpc, client, done) { if (per_rpc) { updateMetadata('', {}, makeTestCall); } else { - client.$_updateMetadata = updateMetadata; + client.$updateMetadata = updateMetadata; makeTestCall(null, {}); } }); diff --git a/src/client.js b/src/client.js index 53b7cc3e..d6b7f59d 100644 --- a/src/client.js +++ b/src/client.js @@ -275,7 +275,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { emitter.getPeer = function getPeer() { return call.getPeer(); }; - this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -349,7 +349,7 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { metadata = metadata.clone(); } var stream = new ClientWritableStream(call, serialize); - this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); callback(error); @@ -425,7 +425,7 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { metadata = metadata.clone(); } var stream = new ClientReadableStream(call, deserialize); - this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -503,7 +503,7 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { metadata = metadata.clone(); } var stream = new ClientDuplexStream(call, serialize, deserialize); - this.$_updateMetadata(this.$_auth_uri, metadata, function(error, metadata) { + this.$updateMetadata(this.$auth_uri, metadata, function(error, metadata) { if (error) { call.cancel(); stream.emit('error', error); @@ -594,43 +594,16 @@ exports.makeClientConstructor = function(methods, serviceName) { options = {}; } options['grpc.primary_user_agent'] = 'grpc-node/' + version; + /* Private fields use $ as a prefix instead of _ because it is an invalid + * prefix of a method name */ this.$channel = new grpc.Channel(address, credentials, options); // Remove the optional DNS scheme, trailing port, and trailing backslash address = address.replace(/^(dns:\/{3})?([^:\/]+)(:\d+)?\/?$/, '$2'); - this.$_server_address = address; - this.$_auth_uri = 'https://' + this.server_address + '/' + serviceName; - this.$_updateMetadata = updateMetadata; + this.$server_address = address; + this.$auth_uri = 'https://' + this.server_address + '/' + serviceName; + this.$updateMetadata = updateMetadata; } - /** - * Wait for the client to be ready. The callback will be called when the - * client has successfully connected to the server, and it will be called - * with an error if the attempt to connect to the server has unrecoverablly - * failed or if the deadline expires. This function will make the channel - * start connecting if it has not already done so. - * @param {(Date|Number)} deadline When to stop waiting for a connection. Pass - * Infinity to wait forever. - * @param {function(Error)} callback The callback to call when done attempting - * to connect. - */ - Client.prototype.$waitForReady = function(deadline, callback) { - var self = this; - var checkState = function(err) { - if (err) { - callback(new Error('Failed to connect before the deadline')); - } - var new_state = self.$channel.getConnectivityState(true); - if (new_state === grpc.connectivityState.READY) { - callback(); - } else if (new_state === grpc.connectivityState.FATAL_FAILURE) { - callback(new Error('Failed to connect to server')); - } else { - self.$channel.watchConnectivityState(new_state, deadline, checkState); - } - }; - checkState(); - }; - _.each(methods, function(attrs, name) { var method_type; if (_.startsWith(name, '$')) { @@ -660,6 +633,44 @@ exports.makeClientConstructor = function(methods, serviceName) { return Client; }; +/** + * Return the underlying channel object for the specified client + * @param {Client} client + * @return {Channel} The channel + */ +exports.getClientChannel = function(client) { + return client.$channel; +}; + +/** + * Wait for the client to be ready. The callback will be called when the + * client has successfully connected to the server, and it will be called + * with an error if the attempt to connect to the server has unrecoverablly + * failed or if the deadline expires. This function will make the channel + * start connecting if it has not already done so. + * @param {Client} client The client to wait on + * @param {(Date|Number)} deadline When to stop waiting for a connection. Pass + * Infinity to wait forever. + * @param {function(Error)} callback The callback to call when done attempting + * to connect. + */ +exports.waitForClientReady = function(client, deadline, callback) { + var checkState = function(err) { + if (err) { + callback(new Error('Failed to connect before the deadline')); + } + var new_state = client.$channel.getConnectivityState(true); + if (new_state === grpc.connectivityState.READY) { + callback(); + } else if (new_state === grpc.connectivityState.FATAL_FAILURE) { + callback(new Error('Failed to connect to server')); + } else { + client.$channel.watchConnectivityState(new_state, deadline, checkState); + } + }; + checkState(); +}; + /** * Creates a constructor for clients for the given service * @param {ProtoBuf.Reflect.Service} service The service to generate a client diff --git a/test/surface_test.js b/test/surface_test.js index 6bb90a23..d917c7a1 100644 --- a/test/surface_test.js +++ b/test/surface_test.js @@ -151,7 +151,7 @@ describe('Client constructor building', function() { }, /\$/); }); }); -describe('Client#$waitForReady', function() { +describe('waitForClientReady', function() { var server; var port; var Client; @@ -169,13 +169,13 @@ describe('Client#$waitForReady', function() { server.forceShutdown(); }); it('should complete when called alone', function(done) { - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); done(); }); }); it('should complete when a call is initiated', function(done) { - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); done(); }); @@ -184,19 +184,19 @@ describe('Client#$waitForReady', function() { }); it('should complete if called more than once', function(done) { done = multiDone(done, 2); - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); done(); }); - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); done(); }); }); it('should complete if called when already ready', function(done) { - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); - client.$waitForReady(Infinity, function(error) { + grpc.waitForClientReady(client, Infinity, function(error) { assert.ifError(error); done(); }); @@ -444,7 +444,8 @@ describe('Other conditions', function() { server.forceShutdown(); }); it('channel.getTarget should be available', function() { - assert.strictEqual(typeof client.$channel.getTarget(), 'string'); + assert.strictEqual(typeof grpc.getClientChannel(client).getTarget(), + 'string'); }); describe('Server recieving bad input', function() { var misbehavingClient;