Skip to content

Conversation

@tvytlx
Copy link

image

When the client sends requests, the process on the server side will block until all hook functions finished, so the client won't get timely respond (sometimes we run long shell scripts in the hook to deploy) and can't figure out what happen on the server either. In fact, we don't need the respond data, right? why not let it return in time.

Maybe we need further discussion.

When the client's request comes, the process on server side will block until all hook functions finished, so the client won't get timely respond and can't figure out what happen on server either. Signed-Off-By: Xiao Tan <tvytlx@gmail.com>
@fophillips
Copy link
Contributor

This looks good. Have you been using it successfully? Might try running this on our production webhooks to see if we have any problems before merging.

@tvytlx
Copy link
Author

@fophillips Yes, It works well in my project.

@fophillips
Copy link
Contributor

@tvytlx have you tried passing threaded=True or processes=2 to app.run instead? I think Werkzeug will support this out of the box.

@tvytlx
Copy link
Author

@fophillipsthreaded or processes is set to serve multiple clients(requests), but the problem here is the single request timeout. Though passing threaded=True, the request wait in a new thread, will still timeout.

@josegonzalez
Copy link

@fophillips any luck trying this out? Just curious if we should wait till this is merged or not :)

@fophillips
Copy link
Contributor

The problem I have with this is if the hook fails there is no feedback 😒

@fophillips
Copy link
Contributor

Could we make it optional instead? Add another decorator maybe for @webhook.async_hook?

@alexchamberlain
Copy link
Contributor

I tend to agree with @fophillips; I'm a little concerned that this obscures the normal case that you want feedback if the hook fails. I could get on board with a second decorator, though.

@tvytlx
Copy link
Author

@alexchamberlain 👌

@tvytlxtvytlx closed this Jul 26, 2017
Sign up for freeto 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

@tvytlx@fophillips@josegonzalez@alexchamberlain