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.

Author: joeyguerra

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 () => {

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,19 +386,22 @@ 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}`)
         }
       }
     } 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

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 () => {

test/doubles/DummyAdapter.mjs

@@ -45,7 +45,7 @@ export class DummyAdapter extends Adapter {
   }
 }
 export default {
-  use (robot) {
+  async use (robot) {
     return new DummyAdapter(robot)
   }
 }