bryancall commented on code in PR #13116:
URL: https://github.com/apache/trafficserver/pull/13116#discussion_r3235708047
##########
src/proxy/http/HttpSM.cc:
##########
@@ -1687,9 +1687,37 @@ HttpSM::handle_api_return()
switch (t_state.next_action) {
case HttpTransact::StateMachineAction_t::TRANSFORM_READ: {
- HttpTunnelProducer *p = setup_transfer_from_transform();
- perform_transform_cache_write_action();
- tunnel.tunnel_run(p);
+ // A plugin has installed an internal response body
(TSHttpTxnErrorBodySet()).
+ // In this branch we bypass transform streaming and switch to internal
transfer.
+ if (t_state.internal_msg_buffer && !t_state.api_server_request_body_set &&
t_state.hdr_info.server_response.valid()) {
+ SMDbg(dbg_ctl_http, "plugin set internal body, bypassing response
transform for internal transfer");
+ t_state.api_info.cache_untransformed = true;
+ // If a tunnel was already set up for transform I/O, shut it down before
we re-route.
+ if (tunnel.is_tunnel_active()) {
+ tunnel.kill_tunnel();
+ }
+ // Drop transform VC table state because this path no longer drives
transform reads.
+ if (transform_info.entry != nullptr) {
+ vc_table.cleanup_entry(transform_info.entry);
+ transform_info.entry = nullptr;
+ }
+ transform_info.vc = nullptr;
+ // Some downstream paths still read client_response; seed it from
transform_response when missing.
+ if (t_state.hdr_info.client_response.valid() == 0 &&
t_state.hdr_info.transform_response.valid()) {
+ t_state.hdr_info.client_response.create(HTTPType::RESPONSE);
+
t_state.hdr_info.client_response.copy(&t_state.hdr_info.transform_response);
+ }
+ // The server session is not needed for internal body transfer if it was
never tunneled.
+ if (server_entry != nullptr && server_entry->in_tunnel == false) {
+ release_server_session();
+ }
+ // Serve the plugin-provided body through the internal tunnel handler.
Review Comment:
Kept this one — function name says what, comment notes WHY the
plugin-provided body is being served through the internal tunnel handler. Open
to dropping if you'd rather; the surrounding block context makes it clear
enough that I could go either way.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -1722,6 +1750,17 @@ HttpSM::handle_api_return()
}
setup_blind_tunnel(true, initial_data);
+ } else if (t_state.internal_msg_buffer &&
!t_state.api_server_request_body_set &&
t_state.hdr_info.server_response.valid() &&
+ plugin_tunnel == nullptr &&
+ (api_hooks.get(TS_HTTP_READ_RESPONSE_HDR_HOOK) != nullptr ||
+ api_hooks.get(TS_HTTP_SEND_RESPONSE_HDR_HOOK) != nullptr)) {
+ // A plugin replaced the origin response body via
TSHttpTxnErrorBodySet().
Review Comment:
Compressed to one line alongside dropping the hook sub-clause in d8ef38db29:
`// Plugin replaced the origin response body via TSHttpTxnErrorBodySet();
divert to internal transfer.`
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7578,12 +7617,21 @@
HttpSM::setup_client_request_plugin_agents(HttpTunnelProducer *p, int num_header
inline void
HttpSM::transform_cleanup(TSHttpHookID hook, HttpTransformInfo *info)
{
+ // Internal-body bypass can skip transform tunnel setup, leaving no transform
+ // entry to clean up. In that case there is nothing safe/useful to close
here.
Review Comment:
Pre-existing — predates this PR. Same wording at HttpSM.cc:7595-7596 too
(response plugin agents twin). Out of scope here; happy to do a separate
cleanup PR if you'd like.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7578,12 +7617,21 @@
HttpSM::setup_client_request_plugin_agents(HttpTunnelProducer *p, int num_header
inline void
HttpSM::transform_cleanup(TSHttpHookID hook, HttpTransformInfo *info)
{
+ // Internal-body bypass can skip transform tunnel setup, leaving no transform
+ // entry to clean up. In that case there is nothing safe/useful to close
here.
+ if (info->entry == nullptr) {
+ return;
+ }
APIHook *t_hook = api_hooks.get(hook);
if (t_hook && info->vc == nullptr) {
do {
- VConnection *t_vcon = t_hook->m_cont;
- t_vcon->do_io_close();
- t_hook = t_hook->m_link.next;
+ APIHook *next = t_hook->m_link.next;
+ // Some transform hooks can already be detached by the time kill_this()
runs.
+ // Guard against null continuations while still draining the remaining
hooks.
Review Comment:
Kept the first line (`// Some transform hooks can already be detached by the
time kill_this() runs.`) — it's load-bearing for the `t_vcon != nullptr` guard.
Removed the second `// Guard against null continuations...` line in d8ef38db29.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7664,7 +7712,14 @@ HttpSM::kill_this()
// In that case, we need to manually close all the
// transforms to prevent memory leaks (INKqa06147)
if (hooks_set) {
- transform_cleanup(TS_HTTP_RESPONSE_TRANSFORM_HOOK, &transform_info);
+ bool bypassed_response_transform =
+ t_state.api_info.cache_untransformed && t_state.internal_msg_buffer &&
!t_state.api_server_request_body_set;
+ // If we intentionally bypassed response transforms for internal-body
+ // transfer, transform_info may be partially detached; skip cleanup in
this
Review Comment:
That 3-line comment was introduced in `6f2dfda5e7` which got dropped in a
force-push earlier today (cleaning up unrelated commits that had ended up on
this branch). The current HEAD has just the `bypassed_response_transform` bool
with no comment. Nothing to do here.
##########
tests/prepare_proxy_verifier.sh:
##########
@@ -40,7 +40,7 @@ pv_dir="${pv_name}-${pv_version}"
pv_tar_filename="${pv_dir}.tar.gz"
pv_tar="${pv_top_dir}/${pv_tar_filename}"
pv_tar_url="https://ci.trafficserver.apache.org/bintray/${pv_tar_filename}";
-expected_sha1="e11b5867a56c5ffd496b18c901f1273e9c120a47"
+expected_sha1="0a60c646cbc9326abb2fbc397cb9efa8c08a807a"
Review Comment:
Agreed — unintentional local-dev drift, not needed upstream. Reverted to the
master SHA1 (`e11b5867...`) in d8ef38db29.
--
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]
