Fix issue with local mode bodiless responses causing clients to hang #540
Conversation
Codecov Report
@@ Coverage Diff @@
## master #540 +/- ##
=========================================
+ Coverage 94.39% 94.4% +<.01%
=========================================
Files 18 18
Lines 3107 3108 +1
Branches 397 397
=========================================
+ Hits 2933 2934 +1
Misses 126 126
Partials 48 48
Continue to review full report at Codecov.
|
CHANGELOG.rst
Outdated
| @@ -2,6 +2,13 @@ | |||
| CHANGELOG | |||
| ========= | |||
|
|
|||
| Next Release (TBD) | |||
| ================== | |||
| * Fix issue with chalice local mode causing http clients to hang on responses | |||
There was a problem hiding this comment.
Line between header and changelog entry.
| @@ -0,0 +1,93 @@ | |||
| import socket | |||
There was a problem hiding this comment.
Hmm, I'm surprised flake8 didn't complain about this. Please use pep8 guidelines for grouping imports.
chalice/local.py
Outdated
| @@ -201,7 +201,7 @@ class NotAuthorizedError(LocalGatewayException): | |||
|
|
|||
|
|
|||
| class NoOptionsRouteDefined(LocalGatewayException): | |||
| CODE = 403 | |||
| CODE = 200 | |||
There was a problem hiding this comment.
Because thats what API gateway does. I missed it before but I noticed it when going over these the tests for cors cases. I can do this as a separate PR if you would prefer.
There was a problem hiding this comment.
I don't see a test for it. I don't mind it in this PR, but we should have a test for this. Might also be good to have a separate commit for this with a description of why it's being changed.
There was a problem hiding this comment.
The existing tests should have covered it and I corrected the tests while I was there anyway and thats when I noticed it was incorrect.
I added these assertions:
There was a problem hiding this comment.
I don't follow. This is updating the code for an exception right? Those tests are for successful requests aren't they? Are you saying we were previously returning a 403 response when we should have been returning a 200?
There was a problem hiding this comment.
The exception is raised when there is no OPTIONS method defined on a route which LocalGateway exceptions are raised when the routes cannot process the request, so that is reasonable so far. The calling code can still generate the OPTIONS request that API Gateway would return and then send a successful response if there is a CORSConfig object on it. I missed that it was a success case somehow before. But looking at it again it seems like it should be decoupled entirely from that type of error since all the others exceptions like this are actual error cases. Whereas this one should be caught and turned into a successful response by auto-generating the CORS headers.
I will fix this in a separate PR.
stealthycoin
force-pushed
the
fix-local-options-hang
branch
from
4d3d771 to
361bf63
Compare
stealthycoin
force-pushed
the
fix-local-options-hang
branch
from
887d339 to
7c1e32f
Compare
fixes #525