Copilot commented on code in PR #13751:
URL: https://github.com/apache/trafficserver/pull/13751#discussion_r4147244396
##########
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) {
- // DEBUG_LOG("shutting down send to server pipe");
- TSVConnShutdown(data->m_upstream.m_vc, 0, 1);
}
Review Comment:
This `else if` branch now intentionally does nothing, but it still matches
events indefinitely as long as `m_upstream.m_write` remains open. Consider
explicitly marking the write stream/VIO as complete/closed in the plugin’s
state (without calling `TSVConnShutdown(..., 0, 1)`), or remove the branch
entirely and rely on other event handling. This makes the control flow clearer
and reduces the risk of future changes accidentally depending on
`m_write.isOpen()` after header send.
##########
plugins/slice/HttpHeader.h:
##########
@@ -115,6 +115,7 @@ struct HttpHeader {
bool hasKey(char const *const key, int const keylen) const;
+ // removes every field (including duplicates) with this key.
// returns false if header invalid or something went wrong with removal.
Review Comment:
Nit: This comment reads like a sentence fragment and starts lowercase.
Consider rephrasing to something like “Removes all fields (including
duplicates) matching this key.” (and optionally clarify that the function
returns false if any removal fails).
##########
plugins/slice/prefetch.cc:
##########
@@ -159,7 +159,9 @@ BgBlockFetch::handler(TSCont contp, TSEvent event, void *
/* edata ATS_UNUSED */
switch (event) {
case TS_EVENT_VCONN_WRITE_COMPLETE:
- TSVConnShutdown(bg->m_stream.m_vc, 0, 1);
+ // The complete request header is sufficient for the background GET.
+ // Keep the write side open so the internal transaction does not receive
+ // an EOS and interpret it as a client abort.
Review Comment:
On `TS_EVENT_VCONN_WRITE_COMPLETE`, the handler now no-ops. Even if the VC
write side must remain open, it’s typically helpful to update internal state to
reflect that no further writes will occur (e.g., close/disable the write
VIO/stream abstraction, release any write buffers if applicable). This avoids
leaving the write path logically ‘open’ in the object model and makes later
cleanup logic less error-prone.
--
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]
