Copilot commented on code in PR #13751:
URL: https://github.com/apache/trafficserver/pull/13751#discussion_r4146840145
##########
plugins/slice/client.cc:
##########
@@ -93,6 +93,12 @@ handle_client_req(TSCont contp, TSEvent event, Data *const
data)
header.setKeyVal(TS_MIME_FIELD_HOST, TS_MIME_LEN_HOST, data->m_hostname,
data->m_hostlen);
+ // Slice never sends a request body on the internal connection. Drop any
+ // body-declaring fields so the internal transaction does not wait for a
+ // body that will never arrive.
+ header.removeKey(TS_MIME_FIELD_CONTENT_LENGTH, TS_MIME_LEN_CONTENT_LENGTH);
+ header.removeKey(TS_MIME_FIELD_TRANSFER_ENCODING,
TS_MIME_LEN_TRANSFER_ENCODING);
+
Review Comment:
`HttpHeader::removeKey(...)` returns a boolean, but these calls ignore the
result. If either removal fails, the internal request can still carry
body-declaring headers and reintroduce the stall this PR is addressing.
Consider checking the return values and failing the transaction early or
logging/debugging when removal fails.
##########
plugins/slice/intercept.cc:
##########
@@ -59,11 +59,10 @@ intercept_hook(TSCont contp, TSEvent event, void *edata)
TSVConnShutdown(data->m_dnstream.m_vc, 1, 0);
}
}
- // server wants more data from us, should never happen
- // every time TSHttpConnect is called this resets
+ // The complete request header is sufficient for GET, HEAD, and PURGE.
+ // Keep the write side open so the internal transaction does not receive
+ // an EOS and interpret it as a client abort.
else if (data->m_upstream.m_write.isOpen() && edata ==
data->m_upstream.m_write.m_vio) {
Review Comment:
This branch is now intentionally a no-op, but the empty block is easy to
misread as incomplete logic. Consider adding an explicit `/* intentionally
no-op */` comment inside the block (or an explicit state transition such as
marking the write VIO closed while keeping the VC open) to make the intent
clear and to avoid tool/linter warnings about empty bodies.
##########
plugins/slice/HttpHeader.cc:
##########
@@ -155,10 +155,13 @@ HttpHeader::removeKey(char const *const keystr, int const
keylen)
bool status = true;
- TSMLoc const locfield = TSMimeHdrFieldFind(m_buffer, m_lochdr, keystr,
keylen);
- if (nullptr != locfield) {
- int const rcode = TSMimeHdrFieldRemove(m_buffer, m_lochdr, locfield);
- status = (TS_SUCCESS == rcode);
+ while (status) {
+ TSMLoc const locfield = TSMimeHdrFieldFind(m_buffer, m_lochdr, keystr,
keylen);
+ if (nullptr == locfield) {
+ break;
+ }
+
+ status = (TS_SUCCESS == TSMimeHdrFieldRemove(m_buffer, m_lochdr,
locfield));
TSHandleMLocRelease(m_buffer, m_lochdr, locfield);
}
Review Comment:
This removes duplicates by repeatedly calling `TSMimeHdrFieldFind(...)`,
which can become O(n²) if many duplicates exist. A more efficient approach is
to find the first matching field once and iterate duplicates (e.g., via the
MIME field 'next duplicate' APIs) while removing them, which keeps the
operation closer to O(n).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
