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

max-concurrency hook property support #167

Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

max-concurrency hook property support #167

wants to merge 4 commits into from

Conversation

ivanpesin
Copy link
Contributor

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.

Copy link
Owner

@adnanh adnanh left a comment

Choose a reason for hiding this comment

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

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.

hook/hook.go Outdated Show resolved Hide resolved
webhook.go Outdated Show resolved Hide resolved
webhook.go Show resolved Hide resolved
webhook.go Outdated Show resolved Hide resolved
@ivanpesin
Copy link
Contributor Author

ivanpesin commented Sep 11, 2017

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
Copy link
Owner

adnanh commented Sep 14, 2017

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ivanpesin
Copy link
Contributor Author

ivanpesin commented Oct 11, 2017

@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
Copy link
Owner

adnanh commented Oct 17, 2017

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
Copy link
Owner

adnanh commented Nov 4, 2017

Hey, I left you a comment on the #175

@PhoenixPeca
Copy link

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

@slightly-blue
Copy link

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants