use EventletTimeoutMiddleware if using eventlet
#4209
+13
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
risicle
force-pushed
the
ris-eventlet-timeout-utils
branch
2 times, most recently
from
d94fef5
to
f28883a
Compare
also catch EventletTimeout exceptions, turning these into 504s, which are the closest thing to an application timeout standard codes seem to cover 60s timeout to start with as this matches our cloudfront/ALB timeouts and so should present the same behaviour to external users
risicle
force-pushed
the
ris-eventlet-timeout-utils
branch
from
f28883a
to
d3cd6fa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See alphagov/notifications-utils#1157
This successfully raises an exception after 30s, which is what we previously expected our gunicorn timeouts to be set to everywhere (and is what they're set to on our non-eventlet gunicorn apps - template-preview-web & antivirus-web). Perhaps we should be setting it to 60s though - what our cloudfront and ALBs supposedly use - because this will cause the same visible behaviour as we currently get.☝️ Changed my mind - switched this to 60s for the reasons suggested above.
I can demonstrate this working on dev-a to anyone interested.
Note that even though it will stop this app processing a request after N seconds, that doesn't mean this cancellation will necessarily be propagated to whatever this app is waiting on (database, another http api..). That sort of thing would require more plumbing.
It would be useful if a developer would check this doesn't break their local development workflow before I merge this. It should have no effect for them because most development modes don't use eventlet by default.