From 3a2b34a2cb9ba13ed93f1cbe68a1fbfc6e8ac219 Mon Sep 17 00:00:00 2001 From: Justin Date: Fri, 6 Mar 2020 12:27:20 -0600 Subject: [PATCH 1/2] fix child process hanging on unclosed connections --- lib/scheduler.js | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/lib/scheduler.js b/lib/scheduler.js index 5320490..5a859e6 100644 --- a/lib/scheduler.js +++ b/lib/scheduler.js @@ -2,7 +2,7 @@ const schedule = require("node-schedule"); const utils = require("./utils"); -const childProcess = require("child_process"); +const path = require('path'); const DEFAULT_TIMEOUT = 6; const MS_PER_SEC = 1000; @@ -81,27 +81,19 @@ class Scheduler { this.serverless.cli.log(`scheduler: scheduling ${fConfig.id}/${eventData.name} ` + `with ${eventData.cron}`); this.serverless.cli.log(`${eventData.name}`); - schedule.scheduleJob(eventData.cron, () => { - this._executeFunction(fConfig.id, eventData.input); - }); + schedule.scheduleJob(eventData.cron, () => this._executeFunction(fConfig, eventData.input)); if (options.runSchedulesOnInit) { - this._executeFunction(fConfig.id, eventData.input); + return this._executeFunction(fConfig.id, eventData.input); } } } return Promise.resolve(); } - _executeFunction(fName, fInput) { - const args = [process.argv[1], "invoke", "local", "--function", fName] - if (fInput) { - args.push("--data", JSON.stringify(fInput)); - } - for (const { name, value } of this._getSlsInvokeOptions()) { - args.push(`--${name}`, value); - } - return childProcess.execFileSync(process.argv[0], args, { cwd: "./", stdio: "inherit" }); + async _executeFunction(fConfig, fInput) { + const modulePath = path.join(__dirname, `../../../${fConfig.moduleName}`); + return require(modulePath)[fConfig.handlerFunction](fInput, this._getContext(fConfig)); } _getSlsInvokeOptions() { @@ -214,7 +206,8 @@ class Scheduler { id: funcName, events: scheduleEvents, timeout: funcConf.timeout, - moduleName: funcConf.handler.split(".")[0] + moduleName: funcConf.handler.split(".")[0], + handlerFunction: funcConf.handler.split(".")[1] }); } } From 9bfc7b1b1b6a29cefb0a3dafe9c11313d3916c3d Mon Sep 17 00:00:00 2001 From: Justin Date: Fri, 6 Mar 2020 14:39:20 -0600 Subject: [PATCH 2/2] refactor and test fix ups --- lib/scheduler.js | 21 +++++---- tests/scheduler.test.js | 95 ++++++++++++++++++++--------------------- tests/test-handler.js | 1 + 3 files changed, 60 insertions(+), 57 deletions(-) create mode 100644 tests/test-handler.js diff --git a/lib/scheduler.js b/lib/scheduler.js index 5a859e6..7f9b7c6 100644 --- a/lib/scheduler.js +++ b/lib/scheduler.js @@ -81,19 +81,24 @@ class Scheduler { this.serverless.cli.log(`scheduler: scheduling ${fConfig.id}/${eventData.name} ` + `with ${eventData.cron}`); this.serverless.cli.log(`${eventData.name}`); - schedule.scheduleJob(eventData.cron, () => this._executeFunction(fConfig, eventData.input)); + schedule.scheduleJob(eventData.cron, () => this._executeFunction(fConfig.id, fConfig.modulePath, fConfig.handlerFunction, fConfig.timeout, eventData.input)); if (options.runSchedulesOnInit) { - return this._executeFunction(fConfig.id, eventData.input); + return this._executeFunction(fConfig.id, fConfig.modulePath, fConfig.handlerFunction, fConfig.timeout, eventData.input); } } } return Promise.resolve(); } - async _executeFunction(fConfig, fInput) { - const modulePath = path.join(__dirname, `../../../${fConfig.moduleName}`); - return require(modulePath)[fConfig.handlerFunction](fInput, this._getContext(fConfig)); + _buildModulePath(configPath) { + // need to back out of the node_modules folder + return path.join(__dirname, `../../../${configPath}`); + } + + _executeFunction(functionId, handlerPath, handlerFunctionName, functionTimeout, fInput) { + const modulePath = this._buildModulePath(handlerPath); + return require(modulePath)[handlerFunctionName](fInput, this._getContext(functionId, functionTimeout)); } _getSlsInvokeOptions() { @@ -120,10 +125,8 @@ class Scheduler { Object.assign(process.env, baseEnv, providerEnvVars, functionEnvVars); } - _getContext(fConfig) { - const functionName = fConfig.id; - - const timeout = fConfig.timeout || this.serverless.service.provider.timeout || DEFAULT_TIMEOUT; + _getContext(functionName, timeout) { + timeout = timeout || this.serverless.service.provider.timeout || DEFAULT_TIMEOUT; const endTime = Math.max(0, Date.now() + timeout * MS_PER_SEC); diff --git a/tests/scheduler.test.js b/tests/scheduler.test.js index 64211e8..8d7acff 100644 --- a/tests/scheduler.test.js +++ b/tests/scheduler.test.js @@ -1,13 +1,12 @@ "use strict"; /* eslint-env mocha */ - -const childProcess = require("child_process"); +const testHandler = require('./test-handler') const Serverless = require("serverless"); const Scheduler = require("../lib/scheduler"); const MS_PER_SEC = 1000; -jest.mock("child_process"); +jest.mock('./test-handler'); describe("validate", () => { let module; @@ -212,22 +211,23 @@ describe("validate", () => { expect(funcs[0].events).toHaveLength(1); const event = funcs[0].events[0]; - module._executeFunction(funcs[0].id, event.input); + module._buildModulePath = () => '../tests/test-handler'; + module._executeFunction(funcs[0].id, funcs[0].moduleName, funcs[0].handlerFunction, funcs[0].timeout, event.input); expect(event.cron).toEqual("1/* * * * *"); expect(event.input.key1).toEqual("value1"); - expect(childProcess.execFileSync).toBeCalledWith( - process.argv[0], - [ - process.argv[1], - "invoke", - "local", - "--function", - funcs[0].id, - "--data", - JSON.stringify(event.input), - ], - {cwd: "./", stdio: "inherit" } + expect(testHandler.test1).toBeCalledWith( + event.input, + expect.objectContaining({ + callbackWaitsForEmptyEventLoop: true, + functionName: funcs[0].id, + functionVersion: "$LATEST", + invokedFunctionArn: `arn:aws:lambda:serverless-offline:123456789012:function:${funcs[0].id}`, + isDefaultFunctionVersion: true, + logGroupName: `/aws/lambda/${funcs[0].id}`, + logStreamName: expect.any(String), + memoryLimitInMB: "1024", + }) ); }); @@ -258,24 +258,23 @@ describe("validate", () => { expect(funcs[0].events).toHaveLength(1); const event = funcs[0].events[0]; - module._executeFunction(funcs[0].id, event.input); + module._buildModulePath = () => '../tests/test-handler'; + module._executeFunction(funcs[0].id, funcs[0].moduleName, funcs[0].handlerFunction, funcs[0].timeout, event.input); expect(event.cron).toEqual("1/* * * * *"); expect(event.input.key1).toEqual("value1"); - expect(childProcess.execFileSync).toBeCalledWith( - process.argv[0], - [ - process.argv[1], - "invoke", - "local", - "--function", - funcs[0].id, - "--data", - JSON.stringify(event.input), - "--option1", - "value2" - ], - {cwd: "./", stdio: "inherit" } + expect(testHandler.test1).toBeCalledWith( + event.input, + expect.objectContaining({ + callbackWaitsForEmptyEventLoop: true, + functionName: funcs[0].id, + functionVersion: "$LATEST", + invokedFunctionArn: `arn:aws:lambda:serverless-offline:123456789012:function:${funcs[0].id}`, + isDefaultFunctionVersion: true, + logGroupName: `/aws/lambda/${funcs[0].id}`, + logStreamName: expect.any(String), + memoryLimitInMB: "1024", + }) ); }); @@ -300,21 +299,21 @@ describe("validate", () => { expect(funcs[0].events).toHaveLength(1); const event = funcs[0].events[0]; - module._executeFunction(funcs[0].id, event.input); + module._buildModulePath = () => '../tests/test-handler'; + module._executeFunction(funcs[0].id, funcs[0].moduleName, funcs[0].handlerFunction, funcs[0].timeout, event.input); expect(event.cron).toEqual("1/* * * * *"); - expect(childProcess.execFileSync).toBeCalledWith( - process.argv[0], - [ - process.argv[1], - "invoke", - "local", - "--function", - funcs[0].id, - "--data", - JSON.stringify(event.input) - ], - { cwd: "./", stdio: "inherit" } + expect(testHandler.test1).toBeCalledWith( + event.input, expect.objectContaining({ + callbackWaitsForEmptyEventLoop: true, + functionName: "scheduled1", + functionVersion: "$LATEST", + invokedFunctionArn: "arn:aws:lambda:serverless-offline:123456789012:function:scheduled1", + isDefaultFunctionVersion: true, + logGroupName: "/aws/lambda/scheduled1", + logStreamName: expect.any(String), + memoryLimitInMB: "1024", + }) ); }); @@ -337,7 +336,7 @@ describe("validate", () => { }; const funcs = module._getFuncConfigs(); - const context = module._getContext(funcs[0]); + const context = module._getContext(funcs[0].id, funcs[0].timeout); expect(context.getRemainingTimeInMillis()).toBeLessThanOrEqual(timeout * MS_PER_SEC); expect(context.getRemainingTimeInMillis()).toBeGreaterThan(timeout * MS_PER_SEC - maxDuration); }); @@ -361,14 +360,14 @@ describe("validate", () => { }; const funcs = module._getFuncConfigs(); - const context = module._getContext(funcs[0]); + const context = module._getContext(funcs[0].id, funcs[0].timeout); expect(context.getRemainingTimeInMillis()).toBeLessThanOrEqual(timeout * MS_PER_SEC); expect(context.getRemainingTimeInMillis()).toBeGreaterThan(timeout * MS_PER_SEC - maxDuration); }); it("should use the *default* timeout for getRemainingTimeInMillis", () => { const timeout = 6; // secs - const maxDuration = 2; // msecs + const maxDuration = 2.1; // msecs module.serverless.service.functions = { scheduled1: { @@ -384,7 +383,7 @@ describe("validate", () => { }; const funcs = module._getFuncConfigs(); - const context = module._getContext(funcs[0]); + const context = module._getContext(funcs[0].id, funcs[0].timeout); expect(context.getRemainingTimeInMillis()).toBeLessThanOrEqual(timeout * MS_PER_SEC); expect(context.getRemainingTimeInMillis()).toBeGreaterThan(timeout * MS_PER_SEC - maxDuration); }); diff --git a/tests/test-handler.js b/tests/test-handler.js new file mode 100644 index 0000000..ebc4910 --- /dev/null +++ b/tests/test-handler.js @@ -0,0 +1 @@ +module.exports.test1 = () => Promise.resolve(); \ No newline at end of file