bryancall commented on code in PR #13116:
URL: https://github.com/apache/trafficserver/pull/13116#discussion_r3235706598
##########
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:
Good catch. Traced `do_api_callout_internal()` (HttpSM.cc:5905) and
confirmed `cur_hook_id` is tautologically `TS_HTTP_READ_RESPONSE_HDR_HOOK` when
we reach `next_action == SERVER_READ` (set unconditionally on that path), and
the `SEND_RESPONSE_HDR_HOOK` arm was dead because `API_SEND_RESPONSE_HDR` never
transitions to `SERVER_READ`. The real guards against TSHttpConnect intercept
regressions are `plugin_tunnel == nullptr` and `server_response.valid()`.
Dropped the entire hook sub-clause in d8ef38db29.
##########
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:
Kept this 2-line comment because it documents a non-obvious cross-API
coupling: `TSHttpTxnErrorBodySet` and `TSHttpTxnServerRequestBodySet` share the
same `internal_msg_buffer`, with `api_server_request_body_set` as the mode
flag. Without the comment, a future reader has no signal that the two TS APIs
are coupled. Took the meta point on comment slop elsewhere — see the rest of
the diff in d8ef38db29.
##########
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:
Compressed the 2-line intro to one line in d8ef38db29: `// Bypass transform
streaming when a plugin replaced the response body.`
##########
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:
Dropped in d8ef38db29. Agreed — code is self-evident.
##########
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:
Compressed to one tight line in d8ef38db29: `// Internal transfer doesn't
use the server session.`
--
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]
