zwoop commented on code in PR #13116:
URL: https://github.com/apache/trafficserver/pull/13116#discussion_r3197036617
##########
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 ||
Review Comment:
Is it even possible to get here, without one of these being true ? What's
the intent here, that the plugin API wasn't called from somewhere else? But
then we wouldn't have a server_response.valid() either.
I don't know for sure, but feels like either this check on the hook is just
superfluous, or possibly should be a assert instead?
##########
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.
Review Comment:
Comment slop, code says exactly the same thing as the comment.
##########
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()).
Review Comment:
Comment slop, code is self explanatory.
##########
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:
Comment slop.
##########
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:
The first comment seems useful, the second is slop.
##########
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:
Comment slop
##########
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.
Review Comment:
Comment slop.
##########
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:
Comment slop.
##########
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:
Partial comment slop, can be reduced to one line.
##########
src/api/InkAPI.cc:
##########
@@ -4948,6 +4948,9 @@ TSHttpTxnErrorBodySet(TSHttpTxn txnp, char *buf, size_t
buflength, char *mimetyp
s->internal_msg_buffer = buf;
s->internal_msg_buffer_size = buf ? buflength : 0;
s->internal_msg_buffer_fast_allocator_size = -1;
+ // TSHttpTxnErrorBodySet() and TSHttpTxnServerRequestBodySet() share the
same buffer.
Review Comment:
Overall, and I'll start that here:
Claude has a big tendency to add comments that were part of its "thinking"
phase. Explaining its reasoning for doing stuff. More often than not, I find
those comments to be completely unnecessary, and often distracting, when the
code itself is self explanatory. In most cases, the comments can be
significantly reduced, and sometimes removed.
I'll refer to this as comment slop :).
##########
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:
What happened here? We bumping proxy verifier version or something as part
of this test suite? Feels like that shouldn't be the case, and if we need to,
should be a separate PR ?
--
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]
