You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/07/20 21:59:15 UTC
[PR] Cache empty chunked responses [trafficserver]
bneradt opened a new pull request, #13410:
URL: https://github.com/apache/trafficserver/pull/13410
Empty chunked responses never start a cache write VIO, so ATS aborts
the cache entry and sends later requests back to the origin.
This starts a zero-byte cache write and preserves the empty-document
state through cache setup. It also adds coverage for negative and
successful responses.
Fixes: #11313
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#issuecomment-5796320677
Cherry-picked to the 10.1.x branch as a6f997b85d6022264e5e7ea1642ed3fe82800ca1 for the 10.1.5 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3617869867
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -210,6 +210,14 @@ CacheVC::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *abuf, bool
vio.ndone = 0;
vio.nbytes = nbytes;
vio.vc_server = this;
+ if (nbytes == 0) {
+ // A zero-byte write represents an empty document, while closing without a
+ // write represents a header-only update.
+ f.allow_empty_doc = 1;
+ if (alternate.valid()) {
Review Comment:
The new `nbytes == 0` handling touches `alternate`/`f.allow_empty_doc` before the DEBUG mutex assertion. If a caller ever violates the locking contract, this reorders the first access to shared state to occur before the diagnostic assert, making failures harder to catch. Consider moving the `#ifdef DEBUG ink_assert(!c || c->mutex->thread_holding);` block above the `nbytes == 0` branch so the lock invariant is checked before accessing `alternate`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3714190355
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,17 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ }
+ } else {
+ c->write_vio = nullptr;
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
do_io_write() is assigned directly to c->write_vio, so a null return already leaves it null. consumer_handler(VC_EVENT_ERROR, c) marks this consumer dead and performs the cache-write cleanup. producer_run() must continue so other consumers are initialized and the common producer/buffer cleanup runs; returning here could strand those consumers and skip releasing buffer_start. This also matches the existing positive-length write failure path, so I’m leaving the control flow unchanged.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3714540004
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,17 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ }
+ } else {
+ c->write_vio = nullptr;
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
In the `c_write == 0` branch, cache writes no longer invoke `consumer_handler(VC_EVENT_WRITE_COMPLETE, c)` on the success path. Previously, the tunnel always notified the consumer of completion for a zero-byte write; now the cache-write path only signals on error. If the cache VC does not independently generate a completion event for a 0-byte VIO, the tunnel may stall waiting for completion/cleanup. Consider calling `consumer_handler(VC_EVENT_WRITE_COMPLETE, c)` after successfully creating the zero-byte cache write VIO (while keeping the VIO creation so that close commits metadata).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3715091775
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,17 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ }
+ } else {
+ c->write_vio = nullptr;
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
CacheVC does independently generate completion for a zero-byte VIO. do_io_write() schedules the cache VC, and CacheVC::openWriteMain() calls calluser(VC_EVENT_WRITE_COMPLETE) when vio.ntodo() <= 0. Calling consumer_handler() synchronously here would cause the completion to be delivered twice and could hit its c->alive assertion. The negative-caching AuTest also passes with the current asynchronous flow, so no change is needed.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3708927405
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,19 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ } else if (c->write_vio->ntodo() == 0 && c->alive) {
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
In the CACHE_WRITE + `c_write == 0` path, calling `consumer_handler(VC_EVENT_WRITE_COMPLETE, c)` is likely to fire twice: `CacheVC::openWriteMain()` will also call back the continuation with `VC_EVENT_WRITE_COMPLETE` when `vio.ntodo() <= 0`. If the tunnel marks the consumer dead here, the later cache callback can hit `ink_assert(c->alive == true)` in `HttpTunnel::consumer_handler()`.
For cache writes, let the cache VC deliver `VC_EVENT_WRITE_COMPLETE` (or `VC_EVENT_ERROR`) via the normal VIO callback instead of calling `consumer_handler()` directly.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3708496221
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -210,6 +210,14 @@ CacheVC::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *abuf, bool
vio.ndone = 0;
vio.nbytes = nbytes;
vio.vc_server = this;
+ if (nbytes == 0) {
+ // A zero-byte write represents an empty document, while closing without a
+ // write represents a header-only update.
+ f.allow_empty_doc = 1;
+ if (alternate.valid()) {
Review Comment:
Good point. I moved the debug mutex assertion above the zero-byte handling so the locking invariant is checked before accessing alternate or f.allow_empty_doc.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#issuecomment-5196009627
Cherry-picked to the 10.2.x branch as e7a6e4448a089daa6f111815ddbf17f68fa6d52b for the 10.2.0 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3709117055
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,19 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ } else if (c->write_vio->ntodo() == 0 && c->alive) {
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
Agreed. CacheVC::openWriteMain() already reports VC_EVENT_WRITE_COMPLETE when the zero-byte VIO has no remaining work, so I removed the synchronous tunnel callback to avoid double delivery. The replay now allows the normal asynchronous cache commit three seconds before follow-up requests.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache empty chunked responses [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13410:
URL: https://github.com/apache/trafficserver/pull/13410#discussion_r3713890740
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1109,9 +1109,17 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
}
if (c_write == 0) {
- // Nothing to do, call back the cleanup handlers
- c->write_vio = nullptr;
- consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ // Cache writes need a VIO even when the body is empty so that closing the
+ // cache VC commits the response metadata instead of aborting the write.
+ if (c->vc_type == HttpTunnelType_t::CACHE_WRITE) {
+ c->write_vio = c->vc->do_io_write(this, 0, c->buffer_reader);
+ if (c->write_vio == nullptr) {
+ consumer_handler(VC_EVENT_ERROR, c);
+ }
+ } else {
+ c->write_vio = nullptr;
+ consumer_handler(VC_EVENT_WRITE_COMPLETE, c);
+ }
Review Comment:
If `do_io_write(...)` returns `nullptr`, `consumer_handler(VC_EVENT_ERROR, c)` is invoked but `c->write_vio` is left as-is and execution continues past this block. That can lead to confusing state and, depending on what `consumer_handler` does, risks follow-on use of a consumer that may already be cleaning up. Set `c->write_vio = nullptr` before calling the error handler and return/stop further processing from `producer_run` after delivering the error event.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org