From 2e0f637b5917e484f5a0a26379900e49f84da7c4 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 29 Jun 2024 16:05:13 +1200 Subject: [PATCH 1/3] Update Http::Stream::reply to Pointer Refactor to avoid storing a raw-pointer, with missing reference-counting. --- src/http/Stream.cc | 54 +++++++++++++++++---------------------- src/http/Stream.h | 9 +++---- src/tests/stub_libhttp.cc | 4 +-- 3 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/http/Stream.cc b/src/http/Stream.cc index 83f8329d5af..f1df150fcfa 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -24,7 +24,6 @@ Http::Stream::Stream(const Comm::ConnectionPointer &aConn, ClientHttpRequest *aReq) : clientConnection(aConn), http(aReq), - reply(nullptr), writtenToSocket(0), mayUseConnection_(false), connRegistered_(false) @@ -136,7 +135,7 @@ Http::Stream::getNextRangeOffset() const "; reply " << reply); // XXX: This method is called from many places, including pullData() which - // may be called before prepareReply() [on some Squid-generated errors]. + // may be called before sendStartOfMessage() [on some Squid-generated errors]. // Hence, we may not even know yet whether we should honor/do ranges. if (http->request->range) { @@ -264,11 +263,15 @@ Http::Stream::socketState() } void -Http::Stream::sendStartOfMessage(HttpReply *rep, StoreIOBuffer bodyData) +Http::Stream::sendStartOfMessage(const HttpReplyPointer &rep, StoreIOBuffer bodyData) { - prepareReply(rep); assert(rep); - MemBuf *mb = rep->pack(); + reply = rep; + + if (http->request->range) + buildRangeHeader(); + + MemBuf *mb = reply->pack(); // dump now, so we do not output any body. debugs(11, 2, "HTTP Client " << clientConnection); @@ -377,7 +380,7 @@ Http::Stream::noteSentBodyBytes(size_t bytes) /// \return true when If-Range specs match reply, false otherwise static bool -clientIfRangeMatch(ClientHttpRequest * http, HttpReply * rep) +clientIfRangeMatch(ClientHttpRequest * http, const HttpReplyPointer &reply) { const TimeOrTag spec = http->request->header.getTimeOrTag(Http::HdrType::IF_RANGE); @@ -387,7 +390,7 @@ clientIfRangeMatch(ClientHttpRequest * http, HttpReply * rep) /* got an ETag? */ if (spec.tag.str) { - ETag rep_tag = rep->header.getETag(Http::HdrType::ETAG); + ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG); debugs(33, 3, "ETags: " << spec.tag.str << " and " << (rep_tag.str ? rep_tag.str : "")); @@ -414,31 +417,28 @@ clientIfRangeMatch(ClientHttpRequest * http, HttpReply * rep) // seems to be something better suited to Server logic /** adds appropriate Range headers if needed */ void -Http::Stream::buildRangeHeader(HttpReply *rep) +Http::Stream::buildRangeHeader() { - HttpHeader *hdr = rep ? &rep->header : nullptr; + auto *hdr = &reply->header; const char *range_err = nullptr; HttpRequest *request = http->request; assert(request->range); /* check if we still want to do ranges */ int64_t roffLimit = request->getRangeOffsetLimit(); - auto contentRange = rep ? rep->contentRange() : nullptr; - if (!rep) - range_err = "no [parse-able] reply"; - else if ((rep->sline.status() != Http::scOkay) && (rep->sline.status() != Http::scPartialContent)) + if ((reply->sline.status() != Http::scOkay) && (reply->sline.status() != Http::scPartialContent)) range_err = "wrong status code"; - else if (rep->sline.status() == Http::scPartialContent) + else if (reply->sline.status() == Http::scPartialContent) range_err = "too complex response"; // probably contains what the client needs - else if (rep->sline.status() != Http::scOkay) + else if (reply->sline.status() != Http::scOkay) range_err = "wrong status code"; else if (hdr->has(Http::HdrType::CONTENT_RANGE)) { - Must(!contentRange); // this is a 200, not 206 response + Must(!reply->contentRange()); // this is a 200, not 206 response range_err = "meaningless response"; // the status code or the header is wrong } - else if (rep->content_length < 0) + else if (reply->content_length < 0) range_err = "unknown length"; - else if (rep->content_length != http->storeEntry()->mem().baseReply().content_length) + else if (reply->content_length != http->storeEntry()->mem().baseReply().content_length) range_err = "INCONSISTENT length"; /* a bug? */ /* hits only - upstream CachePeer determines correct behaviour on misses, @@ -446,10 +446,10 @@ Http::Stream::buildRangeHeader(HttpReply *rep) */ else if (http->loggingTags().isTcpHit() && http->request->header.has(Http::HdrType::IF_RANGE) && - !clientIfRangeMatch(http, rep)) + !clientIfRangeMatch(http, reply)) range_err = "If-Range match failed"; - else if (!http->request->range->canonize(rep)) + else if (!http->request->range->canonize(reply.getRaw())) range_err = "canonization failed"; else if (http->request->range->isComplex()) range_err = "too complex range header"; @@ -466,7 +466,7 @@ Http::Stream::buildRangeHeader(HttpReply *rep) http->request->ignoreRange(range_err); } else { /* XXX: TODO: Review, this unconditional set may be wrong. */ - rep->sline.set(rep->sline.version, Http::scPartialContent); + reply->sline.set(reply->sline.version, Http::scPartialContent); // before range_iter accesses const auto actual_clen = http->prepPartialResponseGeneration(); @@ -474,13 +474,13 @@ Http::Stream::buildRangeHeader(HttpReply *rep) const int spec_count = http->request->range->specs.size(); debugs(33, 3, "range spec count: " << spec_count << - " virgin clen: " << rep->content_length); + " virgin clen: " << reply->content_length); assert(spec_count > 0); /* append appropriate header(s) */ if (spec_count == 1) { const auto singleSpec = *http->request->range->begin(); assert(singleSpec); - httpHeaderAddContRange(hdr, *singleSpec, rep->content_length); + httpHeaderAddContRange(hdr, *singleSpec, reply->content_length); } else { /* multipart! */ /* delete old Content-Type, add ours */ @@ -563,14 +563,6 @@ Http::Stream::deferRecipientForLater(clientStreamNode *node, HttpReply *rep, Sto deferredparams.queuedBuffer = receivedData; } -void -Http::Stream::prepareReply(HttpReply *rep) -{ - reply = rep; - if (http->request->range) - buildRangeHeader(rep); -} - /** * Packs bodyData into mb using chunked encoding. * Packs the last-chunk if bodyData is empty. diff --git a/src/http/Stream.h b/src/http/Stream.h index 9708e8e1b97..5378e1e3f52 100644 --- a/src/http/Stream.h +++ b/src/http/Stream.h @@ -103,16 +103,13 @@ class Stream : public RefCountable clientStream_status_t socketState(); /// send an HTTP reply message headers and maybe some initial payload - void sendStartOfMessage(HttpReply *, StoreIOBuffer bodyData); + void sendStartOfMessage(const HttpReplyPointer &, StoreIOBuffer); /// send some HTTP reply message payload void sendBody(StoreIOBuffer bodyData); /// update stream state when N bytes are being sent. /// NP: Http1Server bytes actually not sent yet, just packed into a MemBuf ready void noteSentBodyBytes(size_t); - /// add Range headers (if any) to the given HTTP reply message - void buildRangeHeader(HttpReply *); - clientStreamNode * getTail() const; clientStreamNode * getClientReplyContext() const; @@ -133,7 +130,7 @@ class Stream : public RefCountable Comm::ConnectionPointer clientConnection; ///< details about the client connection socket ClientHttpRequest *http; /* we pretend to own that Job */ - HttpReply *reply; + HttpReplyPointer reply; char reqbuf[HTTP_REQBUF_SZ]; struct { unsigned deferred:1; ///< This is a pipelined request waiting for the current object to complete @@ -160,7 +157,7 @@ class Stream : public RefCountable int64_t writtenToSocket; private: - void prepareReply(HttpReply *); + void buildRangeHeader(); void packChunk(const StoreIOBuffer &bodyData, MemBuf &); void packRange(StoreIOBuffer const &, MemBuf *); void doClose(); diff --git a/src/tests/stub_libhttp.cc b/src/tests/stub_libhttp.cc index 9192b8579f8..74e525bad7a 100644 --- a/src/tests/stub_libhttp.cc +++ b/src/tests/stub_libhttp.cc @@ -111,10 +111,10 @@ int64_t Stream::getNextRangeOffset() const STUB_RETVAL(-1) bool Stream::canPackMoreRanges() const STUB_RETVAL(false) size_t Stream::lengthToSend(Range const &) const STUB_RETVAL(0) clientStream_status_t Stream::socketState() STUB_RETVAL(STREAM_NONE) -void Stream::sendStartOfMessage(HttpReply *, StoreIOBuffer) STUB +void Stream::sendStartOfMessage(const HttpReplyPointer &, StoreIOBuffer) STUB void Stream::sendBody(StoreIOBuffer) STUB void Stream::noteSentBodyBytes(size_t) STUB -void Stream::buildRangeHeader(HttpReply *) STUB +void Stream::buildRangeHeader() STUB clientStreamNode *Stream::getTail() const STUB_RETVAL(nullptr) clientStreamNode *Stream::getClientReplyContext() const STUB_RETVAL(nullptr) ConnStateData *Stream::getConn() const STUB_RETVAL(nullptr) From 944599f91a11235afd9271ad55f0fdc1079df86c Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Mon, 1 Jul 2024 23:25:31 +1200 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Alex Rousskov --- src/http/Stream.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/http/Stream.cc b/src/http/Stream.cc index f1df150fcfa..1b3e3649ba5 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -271,7 +271,7 @@ Http::Stream::sendStartOfMessage(const HttpReplyPointer &rep, StoreIOBuffer body if (http->request->range) buildRangeHeader(); - MemBuf *mb = reply->pack(); + const auto mb = reply->pack(); // dump now, so we do not output any body. debugs(11, 2, "HTTP Client " << clientConnection); @@ -419,7 +419,8 @@ clientIfRangeMatch(ClientHttpRequest * http, const HttpReplyPointer &reply) void Http::Stream::buildRangeHeader() { - auto *hdr = &reply->header; + Assure(reply); + const auto hdr = &reply->header; const char *range_err = nullptr; HttpRequest *request = http->request; assert(request->range); From 3605a0028ce7d3ff6911dcec42a78e7b3193aa48 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Mon, 1 Jul 2024 23:51:08 +1200 Subject: [PATCH 3/3] Upgrade Http::Stream::DeferredParams::reply as well --- src/client_side.cc | 2 +- src/http/Stream.cc | 5 ++--- src/http/Stream.h | 4 ++-- src/tests/stub_libhttp.cc | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index 2ec12ddca1f..7d4f4d6d9f3 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -880,7 +880,7 @@ ClientSocketContextPushDeferredIfNeeded(Http::StreamPointer deferredRequest, Con /** defer now. */ clientSocketRecipient(deferredRequest->deferredparams.node, deferredRequest->http, - deferredRequest->deferredparams.rep, + deferredRequest->deferredparams.reply.getRaw(), deferredRequest->deferredparams.queuedBuffer); } diff --git a/src/http/Stream.cc b/src/http/Stream.cc index 1b3e3649ba5..7116af9db6c 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -33,7 +33,6 @@ Http::Stream::Stream(const Comm::ConnectionPointer &aConn, ClientHttpRequest *aR flags.deferred = 0; flags.parsed_ok = 0; deferredparams.node = nullptr; - deferredparams.rep = nullptr; } Http::Stream::~Stream() @@ -554,13 +553,13 @@ Http::Stream::initiateClose(const char *reason) } void -Http::Stream::deferRecipientForLater(clientStreamNode *node, HttpReply *rep, StoreIOBuffer receivedData) +Http::Stream::deferRecipientForLater(clientStreamNode *node, const HttpReplyPointer &rep, StoreIOBuffer receivedData) { debugs(33, 2, "Deferring request " << http->uri); assert(flags.deferred == 0); flags.deferred = 1; deferredparams.node = node; - deferredparams.rep = rep; + deferredparams.reply = rep; deferredparams.queuedBuffer = receivedData; } diff --git a/src/http/Stream.h b/src/http/Stream.h index 5378e1e3f52..5a66dd2441d 100644 --- a/src/http/Stream.h +++ b/src/http/Stream.h @@ -124,7 +124,7 @@ class Stream : public RefCountable /// terminate due to a send/write error (may continue reading) void initiateClose(const char *reason); - void deferRecipientForLater(clientStreamNode *, HttpReply *, StoreIOBuffer receivedData); + void deferRecipientForLater(clientStreamNode *, const HttpReplyPointer &, StoreIOBuffer receivedData); public: // HTTP/1.x state data @@ -149,7 +149,7 @@ class Stream : public RefCountable public: clientStreamNode *node; - HttpReply *rep; + HttpReplyPointer reply; StoreIOBuffer queuedBuffer; }; diff --git a/src/tests/stub_libhttp.cc b/src/tests/stub_libhttp.cc index 74e525bad7a..c61300df59d 100644 --- a/src/tests/stub_libhttp.cc +++ b/src/tests/stub_libhttp.cc @@ -121,6 +121,6 @@ ConnStateData *Stream::getConn() const STUB_RETVAL(nullptr) void Stream::noteIoError(const Error &, const LogTagsErrors &) STUB void Stream::finished() STUB void Stream::initiateClose(const char *) STUB -void Stream::deferRecipientForLater(clientStreamNode *, HttpReply *, StoreIOBuffer) STUB +void Stream::deferRecipientForLater(clientStreamNode *, const HttpReplyPointer &, StoreIOBuffer) STUB }