Listener Middleware #803
Listener Middleware #803
Conversation
|
Not sure if there is a clean way to create a PR that shows this is dependent on another branch... Code is complete; working on documentation. |
Allow injecting arbitrary code between the Listener match and execute steps. Like in Express, middleware can interrupt the response process, preventing the Listener callback from ever being executed. Middleware can perform operations both on the way towards the Listener callback (before callback execution) and on the way away from the Listener callback (after callback execution/middleware interrupt). As a side effect, listeners are now executed asynchronously. Behavior around message.done should remain the same (process until message.done is true).
michaelansel
force-pushed
the
middleware
branch
from
96ee035 to
02f630f
Compare
|
Documentation complete; ready for review and merge |
michaelansel
force-pushed
the
middleware
branch
from
0847d75 to
e7c5321
Compare
Everything appears to work fine with 0.1.0, so relaxing the version requirement.
| # If not, the middleware should call the 'done' function with | ||
| # no arguments. | ||
| # | ||
| # Returns nothing. |
There was a problem hiding this comment.
provided that @Middleware is an array, the push operation should return the new size of the array (as per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push)?
There was a problem hiding this comment.
You'd probably want to explicitly return null or undefined after that then.
There was a problem hiding this comment.
Got it. I'll change to return undefined.
I'll have to review the changes more, but I'd be reallly concerned about how this might affect existing code. It looks like you are using detectSeries though, which I think means that while the calls are async, they are called in series, so it should preserve existing behavior. |
|
Also wanted to note, I was considering the async module, but was also looking at run-async. I came across it while trying to understand how yeoman-generator works. In particular, this has the benefit of being indifferent to if the callback function returns immediately, or is called asynchronously. Mostly, it gives control to the script writer. |
Correct, detectSeries should maintain the old behavior. Upon further review, I do note that exception handling changes a bit: if anything in the middleware stack yields to the event loop, the stack does reset and listener callback exceptions will trigger an uncaught. I think the best way to address this is:
I'll add another commit with the new try-catch and expected behavior. As a point of experience, we've been running this specific change (copied over from #724) since June and haven't noticed any strange behavior (but we also haven't had any asynchronous middleware in the stack). |
Interesting! Never heard of run-async before. I would be up for changing the script interface to leverage that, but I think we should require basic middleware to stay as-is because middleware must make a decision (continue or cancel). I feel that adding the option to run synchronous might overcomplicate an already non-intuitive interface. That said, I think it would be pretty easy to build a synchronous wrapper for middleware that allows you to return a boolean for continue/cancel instead of hitting a callback. In that situation, you would probably lose access to wrapping the Regarding changing the scripting interface, I'd like to hold off on changing the interface yet. I'm still roughing out ideas, but I'd like to build out the rackup file method of launching and use that as a way to easily deprecate the current script format ( |
Due to the potentially asynchronous nature of middleware, we need to push the try-catch block down to surround just the listener callback. As a result, middleware is responsible for catching its own exceptions and emitting an 'error' event. The original try-catch is left in place to catch any errors in bad synchronous middleware. Not guaranteed to catch it all, but might as well catch some.
|
Fixed! |
In the event of an entire middleware stack that never defers to the event loop (or no middleware at all), an extremely long stack trace builds up (each listener building on the last). Instead, we defer to the event loop after testing each listener, resulting in much more manageable stack sizes. This should not change effective behavior of listener processing unless a script relies on all listeners being evaluated in a single turn of the event loop (which is already really bad).
| @@ -100,6 +100,10 @@ It wouldn't be called for: | |||
| * has anyone ever mentioned how lovely you are when you open pod bay doors? | |||
| * because it lacks the robot's name | |||
|
|
|||
| The options Object is a way to attach arbitrary metadata to a Listener (hear/respond entry) and enable easy extension of the core hubot functionality. By default, the only handled option inside the options Object is `id`. Additional options may be handled by other scripts that extend hubot; for more information, see the Middleware section of this document. | |||
There was a problem hiding this comment.
You can make 'Middleware section' a link to #middleware, which should just work when viewing it.
|
I've been digging into this code a lot more today, and the part I'm having a trouble with is Listener's So, what I was thinking is that the |
|
Doing some more reading, just series has the same control flow (ie invoke next callback with error to stop going to the next thing). Using waterfall might be better, as you'd be able to manipulate the arguments passed into a tasks' callback, and then pass them into the next task. At the end, the listener would be invoked with whatever the last script sent to it. |
Conflicts: docs/scripting.md
Rename function to match convention
|
@michaelansel anything I can do to help this along? We're getting to the point internally where I want to make sure we're not diverging. |
|
Really just stuck on time right now. I haven't had time to think about pretty much anything Hubot related. The only outstanding thing I have is tweaking the interface. I'm thinking |
I think that's fine too.
That sounds solid. I'm a little wary diving into this today, especially with the docs in a separate pull. But if we land this, it'll be much easier for me to jump in and help with receive middleware directly. |
|
Okay, API updated. I read through the docs in michaelansel#2 and it looks like all that is left is either not yet implemented or already covered. Any other thoughts on this before I do a final review pass? I'll see if I can find someone new to give everything a read through... |
This sounds great to me. Excited to get my hands on this! |
|
While I'm getting additional review from coworkers, @technicalpickles do you have any reservations about merging this? |
| @@ -592,7 +592,7 @@ In addition to a regular expression and callback, the `hear` and `respond` funct | |||
|
|
|||
| The most important and most common metadata key is `id`. Every Listener should be given a unique name (options.id; defaults to `null`). Names should be scoped by module (e.g. 'my-module.my-listener'). These names allow other scripts to directly address individual listeners and extend them with additional functionality like authorization and rate limiting. | |||
There was a problem hiding this comment.
Reading through the documentation, I had some thoughts.
- I think it'd be nice to bold "every", as this tool will be much much more useful as external packages are updated.
- It seems like it would be a good idea to mention, that existing packages will need to be updated to be able to take advantage of this. Maybe this particular document isn't the best location, but at least for a while, I would probably be helpful to point out that these features will require support from package authors.
There was a problem hiding this comment.
Good point! Not really a part of middleware though. Would you mind putting together a quick PR, and I'll merge it separately?
There was a problem hiding this comment.
Yeah, I can do that in a bit.
Thanks for all this hard work! It will make my hubot's maintenance muuuuuuuch better!
|
I've done a pretty careful review of the docs. Everything looks in order. Time to push the big green button! |
|
Awesomesauce. Happy almost-birthday, #803!
|
|
Thanks again @michaelansel, and everyone else! 👍 |
|
OMG did this just got merged? Great job everyone :) |
Builds on #801 (57b7220) to allow injecting arbitrary code between the Listener match and execute steps. Like in Express, middleware can interrupt the response process, preventing the Listener callback from ever being executed. Middleware can perform operations both on the way towards the Listener callback (before callback execution) and on the way away from the Listener callback (after callback execution/middleware interrupt).
As a side effect, listeners are now executed asynchronously. Behavior around message.done should remain the same (process until message.done is true).
Example usage (imposes authorization policy on all scripts): https://github.com/michaelansel/hubot-rbac/blob/master/src/rbac.coffee
API Change:
Robot.receiveandListener.callare now asynchronous. Any external code depending on knowing when all Listeners have been tested (inreceive) or when the Listener callback has finished executing (incall) will need to be updated to use the new callback parameter. Code that doesn't care about timing should continue to work without modification.