Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(middleware): Return a promise that resolves when middleware stack done #1339

Closed
wants to merge 11 commits into from
Closed
4 changes: 4 additions & 0 deletions docs/scripting.md
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,10 @@ Every middleware receives the same API signature of `context`, `next`, and
`done`. Different kinds of middleware may receive different information in the
`context` object. For more details, see the API for each type of middleware.

Middleware execution returns a promise that resolves with the final `context`
when the middleware stack completes. If the middleware stack throws an error,
the promise will be rejected with the error and `context` at that point.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my other comment, a promise can only reject with one argument


### Error Handling

For synchronous middleware (never yields to the event loop), hubot will automatically catch errors and emit an an `error` event, just like in standard listeners. Hubot will also automatically call the most recent `done` callback to unwind the middleware stack. Asynchronous middleware should catch its own exceptions, emit an `error` event, and call `done`. Any uncaught exceptions will interrupt all execution of middleware completion callbacks.
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"cline": "^0.8.2",
"coffee-script": "1.6.3",
"connect-multiparty": "^1.2.5",
"es6-promise": "^4.1.0",
"express": "^3.21.2",
"log": "1.4.0",
"optparse": "1.0.4",
Expand Down
57 changes: 35 additions & 22 deletions src/middleware.coffee
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
async = require 'async'
require('es6-promise').polyfill()

class Middleware
# We use this recursively, and using nextTick recursively is deprecated in node 0.10.
Expand All @@ -21,32 +22,44 @@ class Middleware
# done() - Initial (final) completion callback. May be wrapped by
# executed middleware.
#
# Returns nothing
# Returns promise - resolves with context when middleware completes
# Returns before executing any middleware
execute: (context, next, done) ->
done ?= ->
# Execute a single piece of middleware and update the completion callback
# (each piece of middleware can wrap the 'done' callback with additional
# logic).
executeSingleMiddleware = (doneFunc, middlewareFunc, cb) =>
# Match the async.reduce interface
nextFunc = (newDoneFunc) -> cb(null, newDoneFunc or doneFunc)
# Catch errors in synchronous middleware
try
middlewareFunc.call(undefined, context, nextFunc, doneFunc)
catch err
# Maintaining the existing error interface (Response object)
@robot.emit('error', err, context.response)
# Forcibly fail the middleware and stop executing deeper
doneFunc()

# Executed when the middleware stack is finished
allDone = (_, finalDoneFunc) -> next(context, finalDoneFunc)
new Promise (resolve, reject) =>

# Execute each piece of middleware, collecting the latest 'done' callback
# at each step.
process.nextTick =>
async.reduce(@stack, done, executeSingleMiddleware, allDone)
done ?= ->

# Allow each middleware to resolve the promise early if it calls done()
pieceDone = ->
done()
resolve context

# Execute a single piece of middleware and update the completion callback
# (each piece of middleware can wrap the 'done' callback with additional
# logic).
executeSingleMiddleware = (doneFunc, middlewareFunc, cb) =>
# Match the async.reduce interface
nextFunc = (newDoneFunc) -> cb(null, newDoneFunc or doneFunc)
# Catch errors in synchronous middleware
try
middlewareFunc.call(undefined, context, nextFunc, doneFunc)
catch err
# Maintaining the existing error interface (Response object)
@robot.emit('error', err, context.response)
# Forcibly fail the middleware and stop executing deeper
doneFunc()
reject err, context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise.reject() only accepts one argument, you can try it out using

Promise.reject(new Error('foo'), {}).catch(function (error, context) {
  console.log(context)
})
// logs `undefined`

What you could do is set context as property of err before rejecting with it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one. Thanks for that info, I'll do it that way instead.


# Executed when the middleware stack is finished
allDone = (_, finalDoneFunc) ->
next context, finalDoneFunc
resolve context

# Execute each piece of middleware, collecting the latest 'done' callback
# at each step.
process.nextTick =>
async.reduce(@stack, pieceDone, executeSingleMiddleware, allDone)

# Public: Registers new middleware
#
Expand Down
2 changes: 1 addition & 1 deletion src/response.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class Response
# strings - One or more strings to be posted. The order of these strings
# should be kept intact.
#
# Returns nothing
# Returns promise - resolves with context when middleware completes
locked: (strings...) ->
@runWithMiddleware("locked", { plaintext: true }, strings...)

Expand Down
2 changes: 1 addition & 1 deletion src/robot.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ class Robot
#
# cb - Optional callback that is called when message processing is complete
#
# Returns nothing.
# Returns promise - resolves with context when middleware completes
# Returns before executing callback
receive: (message, cb) ->
# When everything is finished (down the middleware stack and back up),
Expand Down
64 changes: 61 additions & 3 deletions test/middleware_test.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,64 @@ describe 'Middleware', ->
allDone
)

it 'returns a promise that resolves when async middleware stack is complete', (testDone) ->

testMiddlewareA = (context, next, done) ->
setTimeout ->
context.A = 'done'
next(done)
, 50

testMiddlewareB = (context, next, done) ->
setTimeout ->
context.B = 'done'
next(done)
, 50

@middleware.register testMiddlewareA
@middleware.register testMiddlewareB

middlewareFinished = ->

middlewarePromise = @middleware.execute(
{}
(_, done) -> done()
middlewareFinished
)

middlewarePromise.then (finalContext) ->
expect(finalContext).to.eql A: 'done', B: 'done'
testDone()

it 'promise resolves when middleware completes early, with context at that point', (testDone) ->

testMiddlewareA = (context, next, done) ->
setTimeout ->
context.A = 'done'
done()
, 50

testMiddlewareB = (context, next, done) ->
setTimeout ->
context.B = 'done'
next(done)
, 50

@middleware.register testMiddlewareA
@middleware.register testMiddlewareB

middlewareFinished = ->

middlewarePromise = @middleware.execute(
{}
(_, done) -> done()
middlewareFinished
)

middlewarePromise.then (finalContext) ->
expect(finalContext).to.eql A: 'done'
testDone()

describe 'error handling', ->
it 'does not execute subsequent middleware after the error is thrown', (testDone) ->
middlewareExecution = []
Expand Down Expand Up @@ -238,7 +296,7 @@ describe 'Middleware', ->
{}
middlewareFinished
middlewareFailed
)
).catch (reason) -> # supress warning re unhandled promise rejection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this start showing unexpected warnings when people upgrade?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without that line 299, it was logging a warning in the tests, where middleware threw. Saying "unhandled promise rejection" - not a big deal, but simply catching the promise and doing nothing with it removes the warning. It would only effect people who's middleware is also throwing, in which case they'd see errors anyway.


it 'emits an error event', (testDone) ->
testResponse = {}
Expand All @@ -263,7 +321,7 @@ describe 'Middleware', ->
{response: testResponse},
middlewareFinished,
middlewareFailed
)
).catch (reason) -> # supress warning re unhandled promise rejection

it 'unwinds the middleware stack (calling all done functions)', (testDone) ->
extraDoneFunc = null
Expand Down Expand Up @@ -291,7 +349,7 @@ describe 'Middleware', ->
{}
middlewareFinished
middlewareFailed
)
).catch (reason) -> # supress warning re unhandled promise rejection

describe '#register', ->
it 'adds to the list of middleware', ->
Expand Down