From cb951f6c57c35d26ff8b643c4a498be397be6750 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 18 Aug 2015 17:38:11 -0700 Subject: [PATCH 1/3] 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 2/3] 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 3/3] 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(); + }); }); });