From a4b688e877c2eb04d14679966c3d55e0b63642a1 Mon Sep 17 00:00:00 2001 From: Joey Guerra Date: Thu, 9 May 2024 11:23:01 -0500 Subject: [PATCH] fix: Scripts making pipelined requests were not waited on to finish before loading the next script. I observed a bug when using Azure app-configuration to get app configuration values from Azure's App Configuration service to create a robot.config object for other scripts to get values from. appConfigClient.getConfigurationSetting pipelines requests and the other scripts (which depended on this one to complete) were being loaded before getConfigurationSetting returned, resulting in a race condition where other scripts failed to load because they depended on robot.config to have the values from App Configuration. --- src/GenHubot.mjs | 8 +++++--- src/Robot.mjs | 23 +++++++++++++++-------- test/XampleTest.mjs | 2 ++ test/doubles/DummyAdapter.mjs | 2 +- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/GenHubot.mjs b/src/GenHubot.mjs index ce673453a..1df97dbd9 100755 --- a/src/GenHubot.mjs +++ b/src/GenHubot.mjs @@ -49,7 +49,7 @@ function runCommands (hubotDirectory, options) { // This is a test script. // -export default (robot) => { +export default async (robot) => { robot.respond(/helo$/, async res => { await res.reply("HELO World! I'm Dumbotheelephant.") }) @@ -92,7 +92,7 @@ export default (robot) => { this.robot.emit('play', envelope, ...strings) } - run () { + async run () { // This is required to get the scripts loaded this.emit('connected') } @@ -108,7 +108,7 @@ export default (robot) => { } } export default { - use (robot) { + async use (robot) { return new DummyAdapter(robot) } } @@ -128,12 +128,14 @@ export default (robot) => { describe('Xample testing Hubot scripts', () => { let robot = null beforeEach(async () => { + process.env.EXPRESS_PORT = 0 robot = new Robot(dummyRobot, true, 'Dumbotheelephant') await robot.loadAdapter() await robot.run() await robot.loadFile('./scripts', 'Xample.mjs') }) afterEach(() => { + delete process.env.EXPRESS_PORT robot.shutdown() }) it('should handle /helo request', async () => { diff --git a/src/Robot.mjs b/src/Robot.mjs index 95e2b5e62..42dc216c2 100644 --- a/src/Robot.mjs +++ b/src/Robot.mjs @@ -332,21 +332,25 @@ class Robot { async loadmjs (filePath) { const forImport = this.prepareForImport(filePath) const script = await import(forImport) + let result = null if (typeof script?.default === 'function') { - script.default(this) + result = await script.default(this) } else { this.logger.warning(`Expected ${filePath} (after preparing for import ${forImport}) to assign a function to export default, got ${typeof script}`) } + return result } async loadjs (filePath) { const forImport = this.prepareForImport(filePath) const script = (await import(forImport)).default + let result = null if (typeof script === 'function') { - script(this) + result = await script(this) } else { this.logger.warning(`Expected ${filePath} (after preparing for import ${forImport}) to assign a function to module.exports, got ${typeof script}`) } + return result } // Public: Loads a file in path. @@ -362,16 +366,17 @@ class Robot { // see https://github.com/hubotio/hubot/issues/1355 if (['js', 'mjs'].indexOf(ext) === -1) { this.logger.debug(`Skipping unsupported file type ${full}`) - return + return null } - + let result = null try { - await this[`load${ext}`](full) + result = await this[`load${ext}`](full) this.parseHelp(full) } catch (error) { this.logger.error(`Unable to load ${full}: ${error.stack}`) throw error } + return result } // Public: Loads every script in the given path. @@ -381,12 +386,14 @@ class Robot { // Returns nothing. async load (path) { this.logger.debug(`Loading scripts from ${path}`) + const results = [] try { const folder = await File.readdir(path, { withFileTypes: true }) for await (const file of folder) { if (file.isDirectory()) continue try { - await this.loadFile(path, file.name) + const result = await this.loadFile(path, file.name) + results.push(result) } catch (e) { this.logger.error(`Error loading file ${file.name} - ${e.stack}`) } @@ -394,6 +401,7 @@ class Robot { } catch (e) { this.logger.error(`Path ${path} does not exist`) } + return results } // Public: Load scripts from packages specified in the @@ -460,7 +468,7 @@ class Robot { if (stat) { app.use(express.static(stat)) } - const p = new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { try { this.server = app.listen(port, address, () => { this.router = app @@ -471,7 +479,6 @@ class Robot { reject(err) } }) - return p } // Setup an empty router object diff --git a/test/XampleTest.mjs b/test/XampleTest.mjs index c13002743..6029cd7c7 100644 --- a/test/XampleTest.mjs +++ b/test/XampleTest.mjs @@ -13,12 +13,14 @@ import dummyRobot from './doubles/DummyAdapter.mjs' describe('Xample testing Hubot scripts', () => { let robot = null beforeEach(async () => { + process.env.EXPRESS_PORT = 0 robot = new Robot(dummyRobot, true, 'Dumbotheelephant') await robot.loadAdapter() await robot.run() await robot.loadFile('./test/scripts', 'Xample.mjs') }) afterEach(() => { + delete process.env.EXPRESS_PORT robot.shutdown() }) it('should handle /helo request', async () => { diff --git a/test/doubles/DummyAdapter.mjs b/test/doubles/DummyAdapter.mjs index 6bf5b93a3..2df061fa0 100644 --- a/test/doubles/DummyAdapter.mjs +++ b/test/doubles/DummyAdapter.mjs @@ -45,7 +45,7 @@ export class DummyAdapter extends Adapter { } } export default { - use (robot) { + async use (robot) { return new DummyAdapter(robot) } }