Fix %err_code/%err_detail for some error:transaction-end-before-headers #264
Conversation
| @@ -81,6 +81,7 @@ typedef enum { | |||
| ERR_REQUEST_START_TIMEOUT, // Aborts the connection instead of error page | |||
| ERR_REQUEST_PARSE_TIMEOUT, // Aborts the connection instead of error page | |||
| ERR_RELAY_REMOTE, // Sends server reply instead of error page | |||
| ERR_STREAM_FAILURE, // No client to send the error page to | |||
There was a problem hiding this comment.
We need a distinct description (currently it duplicates ERR_CLIENT_GONE).
and resort to ERR_STREAM_FAILURE if rawError is missing.
src/client_side.cc
Outdated
| debugs(83, 5, "forgetting client " << intputToConsume << " bytes: " << inBuf.length()); | ||
| inBuf.clear(); | ||
| } else { | ||
| bareError.update(error ? error : ERR_STREAM_FAILURE); // XXX: bareLogTagsErrors |
There was a problem hiding this comment.
This change secretly drops WITH_CLIENT detail for ERR_NONE cases (those cases are converted to ERR_STREAM_FAILURE here). Assuming that drop is intentional/correct, we should adjust this function code (including comments) to make that drop explicit/expected.
There was a problem hiding this comment.
Adjusted. I left the XXX for now - I could not recollect its meaning.
src/client_side.cc
Outdated
| debugs(83, 5, "forgetting client " << intputToConsume << " bytes: " << inBuf.length()); | ||
| inBuf.clear(); | ||
| } else { | ||
| bareError.update(error ? error : ERR_STREAM_FAILURE); // XXX: bareLogTagsErrors |
There was a problem hiding this comment.
Before this change, bareError update logic was fairly simple/clear1: If there are no transactions in the pipeline, then this function does not give error to any transaction, so we should store error in bareError (that will be used later, when creating and logging unparsed transaction, if any).
Now bareError update code is completely ignoring the presence of pipeline transactions (and their use of error) and entangles bareError update with complex logic of determining whether any unparsed transactions exist.
Can we simplify PR changes/code by updating bareError where it was updated before, but doing so unconditionally, leaving complex (and buggy!) code that determines the presence of unparsed transactions intact? If ConnStateData ends up using updated bareError, great! If updated bareError remains unused, it is not a big deal. IIRC, that data member exists "just in case it is needed for future transaction objects".
Footnotes
-
I am not implying that old logic was correct. ↩
|
Opened another PR instead, which does not depend on SQUID-977-do-not-log-twice changes. |
These format codes were empty in some situations for the second (and
subsequent) pipeline transactions if something went wrong with the
previous transaction (e.g., client connection was aborted, server
send an incomplete response body, etc.). Now a new ERR_STREAM_FAILURE
error code marks all these cases.