Repository navigation
Update to refined timeout spec - #463
Conversation
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
stefanvanburen
left a comment
There was a problem hiding this comment.
Looks good to me overall!
One thing I noticed: check_deadline isn't Connect-specific, so a gRPC request with grpc-timeout: 0m (or anything that's already expired by the time we build the context) also short-circuits now. IIUC on main the WSGI server never looked at the deadline at all and would just run the handler to completion, so this seems like a fix rather than a problem (and makes sync match async) — but mind adding a gRPC case to the _client_errors table so it's pinned? (connect-go runs the handler with an already-expired context instead, FWIW, so it's slightly different, but I think skipping it is fine.)
Also, IIUC header-validation errors on Connect streams (bad timeout, bad connect-protocol-version) now come back as a 200 + end-stream error rather than a plain HTTP error, which I think is right, just a bit broader than the PR title.
nit (no strong feelings): the stream response setup in _server_sync.py and _server_async.py is now pretty much line-for-line the same, down to the comment; might be worth pulling the non-I/O bits into _server_shared.py at some point, but happy to punt.
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
|
Sure added grpc test cases. Let's fix forward on the code cleanup, IIRC you were looking at more broadly doing some more code sharing after matching up some corner case behaviors |
Rejects empty connect timeout header and handles
0as per a spec update.In updating the handling, this also refactors WSGI a little to be closer to the control flow of ASGI
connectrpc/connectrpc.com#429
connectrpc/conformance#1193