878668 - Try to enforce a timeout on each individual request

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, defect, P2)

Description

[Ryan Kelly [:rfkelly]]

Attachment: patch to timeout individual requests in the gunicorn worker

The standard gunicorn --timeout argument doesn't work like most people expect.  It's not a timeout for each individual request, but rather for the worker process as a while - as long as *some* activity is taking place inside the worker, the timeout will not be triggered.

This patch extends our custom gunicorn worker subclass to do the expected thing, and apply the timeout to each individual request.

There's some caveats to using gevent.Timeout() for this.  In particular, the timeout won't trigger until you yield to the event loop, so it can't interrupt CPU-bound tasks.  But it does the right thing in most cases.

There's also a bug in PyMySQL that prevents this from working quite as well as I'd like, see https://github.com/petehunt/PyMySQL/pull/148

Caveats aside, I've locally tested that this works in the case we're most interested in right now - if the timeout expires, and we kill the running MySQL query, then the request handler will error out rather than retrying the query.

Comment 1

[Toby Elliott [:telliott]]

Comment on attachment 757207 [details] [diff] [review]
patch to timeout individual requests in the gunicorn worker

Fingers crossed. I dislike that we're trying to solve problems with all the layers of wrapping by adding another wrapper class, but recognize that it may be necessary :P

Comment 2

[Ryan Kelly [:rfkelly]]

(In reply to Toby Elliott [:telliott] from comment #1)
> Comment on attachment 757207 [details] [diff] [review]
> patch to timeout individual requests in the gunicorn worker
> 
> Fingers crossed. I dislike that we're trying to solve problems with all the
> layers of wrapping by adding another wrapper class

Indeed.  Although I like to think of this as tweaking an existing layer so it works a little better, rather than adding more layers to the system :-)

Committed in http://hg.mozilla.org/services/server-core/rev/ab0cff6c4c17

Comment 3

[Ryan Kelly [:rfkelly]]

Re-opening because we need to get this ported over to the sync1.5 stack.

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: gunicorn_worker.py

Here's the same worker code, with license header changed and renamed to "MozSvcGeventWorker" to be slightly more clear what it's all about.  Let's put it in mozsvc and we can try it for our next deploy.

Comment 5

[Toby Elliott [:telliott]]

To clarify: there's no code change in this latest review request, just header stuff? It's not a diff, so I want to be sure.

Comment 6

[Ryan Kelly [:rfkelly]]

I changed the name from `MozSvcWorker` to `MozSvcGeventWorker` for a little more clarity, but otherwise no, on code change from the sync1.1 version.

Comment 7

[Ryan Kelly [:rfkelly]]

Committed in https://github.com/mozilla-services/mozservices/commit/8850a298c1352d9e91bd146305fd38edd4374f35

Now we need to make sure it's actually used in our production config - :jbonacci watch out for this one in the next TS/Sync deploy ticket.

Comment 8

[James Bonacci [:jbonacci]]

We can verify Prod logs I guess.
No way to directly test Prod anymore...

Comment 9

[James Bonacci [:jbonacci]]

:bobm can you help me verify this in Prod?

Comment 10

[James Bonacci [:jbonacci]]

I am going to assume we are all good here, otherwise we would know by now (multiple pushes to Prod)...