chunked disconnect now throws InvalidChunkHeaders#578
Conversation
|
Some tests from conduit failing. Standby... |
|
I think this PR is in good shape now. Some CI tests are failing, but it looks like dependency rot (correct me if I'm wrong) which has nothing to do with this PR. |
|
@goertzenator sorry for the delay, I'll try to get to this eventually. |
- Fixes issue snoyberg#168 - Was previously throwing `IncompleteHeaders` which is confusing and misleading. - Untangles readLine functions and call sites.
|
@goertzenator I may wanna do a small refactoring before changing this code. Would you be willing to rebase your code after I do that (there will be conflicts)? |
|
For sure, go for it. |
There was a problem hiding this comment.
First things first, overall, I like this change.
Your code does two things:
- Refactoring: Effectively getting rid of
connectionReadLineWith, which I like, as it removes some code duplication inconnectionReadLine. However, I'm not sure if it's possible to do that while keeping the existing behavior (see my comment onnextStatusLine). - Fix the exception semantics for chunked body readers. You communicate the failure condition via the return value, which I think in general is preferable. However, in this specific case we always turn
NothingintothrowHttpat the call site. So I think maybe instead pass in the error type, and keep the calls tothrowHttpwhere they currently are. I think doing so, will allow you to do (2) before (1).
To sum things up, this is what I propose:
- One PR that only does (2), addressing the issue by passing through the error type (
HttpExceptionContent). - If you're then still up for it, a separate PR that does (1), the refactoring (remove the code duplication in
connectionReadLine). In that PR we can then solely focus on what to do aboutnextStatusLine.
| connectionReadLineWith mhl conn bs >>= parseStatus mhl 3 | ||
| mbs <- connectionReadLineMaybe mhl conn | ||
| case mbs of | ||
| Nothing -> throwHttp NoResponseDataReceived |
There was a problem hiding this comment.
I think, strictly speaking, this changes existing behavior.
Specifically, if I'm not mistaken, NoResponseDataReceived can now be thrown even after some data has been received (some data without CRLF, then empty chunk).
|
Thanks for taking a close look at it. I'm deep into some other stuff right now, but I hope to get back to it in within a week. |
2 participants
Fixes 11 year old issue #168
We observed a
IncompleteHeadersexception after connecting to a streaming endpoint for an hour with a server initiated disconnect. That is rather misleading, so this PR changes that toInvalidChunkHeaders.The readline functions and call sites were also improved.
All tests pass.