max-concurrency hook property support

[ivanpesin]

I have implemented support for hook concurrency limits (issue #148):

   {
     "id": "sleep",
     "execute-command": "/tmp/sleep-test.bsh",
     "command-working-directory": "/tmp",
     "include-command-output-in-response": true,
     "max-concurrency": 2
   }

When the limit is reached, webhook will return 429 Too Many Requests error and show error message:

[webhook] 2017/08/26 11:02:43 sleep got matched
[webhook] 2017/08/26 11:02:43 reached concurrency limit for: sleep (max=2)
[webhook] 2017/08/26 11:02:43 429 | 114.527µs | localhost:9000 | GET /hooks/sleep 

Let me know if you want it to be implemented differently.

[adnanh]

It's an okay approach. Other one would be storing these executions in the hook object. Usage of channels is creative, but a simple integer property could be more memory efficient :-)

(Don't get me wrong, I don't want you to switch back from channels, let's go with this approach until the end. We'll optimize later if we hit performance/memory issues).

Needs a bit more work, but looking good overall.

[ivanpesin]

Thanks for the feedback!

Hook object looked basically "static" to me: it's only updated when configuration is read, thus I didn't want to put counters in it. It would also involve mutexes to make the counter thread-safe. That's why I decided to go with channels.

I think I've fixed all the issues you pointed out. I've also reduced nesting in reloadHooks and added map cleanup in removeHooks. To see clearly the change to reloadHooks check the diff ignoring whitespaces.

Let me know if any other changes needed.

[adnanh]

Alright, I tried out some scenarios. This works perfect when the include-command-output-in-response is set to true.

However, when that is not the case, max-concurrent-executions flag has no effect because the hookHandler function launches goroutine and exits, executing the defer func() { <-hookExecutions[id] }() before the actual goroutine is done, which in turn allows launching arbitrary number of concurrent executions.

For this to work properly, the handleHook method should actually send the signal to the channel that the hook handling is done, not the hookHandler.

With that in mind, few other things can bite us from the nature of concurrent executions:

• We have to be careful about writing to the channel after the goroutine is finished because hook might have been deleted in the meantime (by hotreload, or the USR1 signal), thus it will try writing in the non existent channel (since the cleanup method deletes the channel for the hook)
• Hook might write to the non existent channel if the hook's maximum concurrent executions setting have been modified (the reload method creates a new channel with the new cap)

[ivanpesin]

Oh, that' right, just yesterday I was looking at this bit of code where handleHook is called and thought there might be an issue. Let me simplify that part a bit as a separate PR and that would also fix the issues you mentioned.

[ivanpesin]

Waiting on #175; if it works, I'll update this PR.

[ivanpesin]

@adnanh -- just an update on this one: I have updated the code in my repository with the assumption #175 is good and tested all the cases you mentioned. Everything seems good and rate limiting works as expected.

When you get few spare minutes, could you review #175 and if it looks good, I'll update this PR with code that accounts for it.

[adnanh]

Hey, didn't forget about you, just busy with work and life. I will grab some time this week to review everything and hopefully merge it :-)

[adnanh]

Hey, I left you a comment on the #175

[PhoenixPeca]

hello, do we have any plans to get this implemented? :D

[slightly-blue]

Hi, would also love this feature. It is pretty vital to my use case. (I'm trying to run gatsby build when changes happen on the CMS but as soon as there are a little bit more updates at once the webhook service gets oom-killed.)