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

Middleware execute returns a promise (v2) #1370

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.

### 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

# 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

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