-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow policies to run in series or parallel #11
Comments
@devinivy So, one thing that MrHorse does is ask each policy to specifically indicate whether handling further policies should happen or not. So you can bail out at any place with a specific error. One thing I'm unclear on is what happens if you are running a few policies in parallel and a couple of them want to respond back with an error. Who wins? So I'm a bit concerned that this might unnecessarily complicate things. Do you have a use case for yourself that needs parallel policies? |
That's a good point! My use-case is just trying to save some time by allowing IO to happen in tandem. I can see why you would want to avoid the added complexity here, but I do have a proposition for how that could work if you decide you're interested. You basically resolve all successes/errors in a set of parallel processes then, to make sure the server behaves deterministically, you take into account the order that they are listed in configuration when choosing the error response. The Items module used by hapi has something that caters to this sort of behavior: https://github.com/hapijs/items#itemsparallelexecutetasks-callback |
Seems like a good change. I'll add it to the queue. |
Cool! Another way to look at this that could achieve the same effect: Ex. var MrHorse = require('mrhorse');
// In route config
{
/* ... */
config: {
policies: [
'loggedInToday',
MrHorse.parallel('isAdmin', 'hasRights', 'smellsGood'),
'moreStuff'
]
}
/* ... */
}
/* or */
// In route config again
{
/* ... */
config: {
policies: [
'loggedInToday',
MrHorse.parallel('isAdmin', 'hasRights', 'smellsGood', function(errs) {
// handle parallel errors in some custom way
}),
'moreStuff'
]
}
/* ... */
} At that point, passing |
I'd be very interested in that approach. And PRs are always welcome! :) |
Not finished by any means, but I've prodded at this in my free time: master...devinivy:master. Any idea what a nice way would be to use default apply point settings when dynamically running a policy? Right now I'm just asserting that a dynamic policy in route options needs to specify its apply point explicitly. |
Looks like a good start. Thanks. On Sun, Jul 5, 2015 at 10:54 PM devin ivy notifications@github.com wrote:
|
Still plugging away at this, and looking for some input if anyone has thoughts. The main issue is that at the time I've opted to sidestep this by allowing policy functions to provide a property that contains a list of policies by which the handler for an apply point can determine if it should run the raw policy that I'm also keeping performance in mind. Do we really want each apply point handler to be spending time trying figure out if it's allowed to run a policy? I suppose it's relatively fast, but I do miss the simplicity and quickness of the policy-apply-point lookup table. Is it possible that we can/should cache the list of policies to run per route? |
Just to make sure I'm clear on what you are thinking, are you wanting to have a lookup table for each route+applyPoint? I'm ok with that. It'd certainly make the handler stupid simple, while the addPolicy function would get a bit more complicated. I definitely am in favor of shifting more processing up front, and less on each request. And are you concerned that you can have a misconfigured route, and we can't notify until the route is used for the first time? So, you'd like to have something more deterministic? I didn't track when you said: "I've opted to sidestep this by allowing policy functions to provide a property that contains a list of policies by which the handler for an apply point can determine if it should run the raw policy that MrHorse.parallel returns." Can you expand/clarify that for me? |
It's a lot easier to see this in the PR I just posted than it is to explain. Basically, now there are two different types of policies: those loaded then referenced in a route config using a string, and those added dynamically into the route config as policy functions. The policies that are loaded are fine! There's already a simple lookup table in
I'm toying with ideas to pre-compute or cache the answers to those two questions. In PR #17 you can see my current solution, which is a little more naive than that. |
I ended-up memoizing the function that determines the apply point based upon a list of policy names. Feels pretty good for now! |
Awesome. I need to focus on a side project tonight so I can't look over On Tue, Jul 7, 2015 at 1:39 PM devin ivy notifications@github.com wrote:
|
Cool! No huge rush on this on my end. I feel like the PR has settled down. It's fully tested, and I'm feeling mostly satisfied, but it definitely needs a second look. |
I've merged in #17. Would you mind doing one more little thing? Would you mind adding a small example to the |
👍 will do. This issue can save as a placeholder for that to be done. |
PR #21 will hopefully round this out! |
It would be nice if we could emulate the functionality from hapi route prerequisites to indicate that policies are allowed to run in parallel or in series when they're part of the same request lifecycle extension.
See
The text was updated successfully, but these errors were encountered: