Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Http::Stream::reply to Pointer #1855

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Upgrade Http::Stream::reply to Pointer #1855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e0f637
Update Http::Stream::reply to Pointer
yadij Jun 29, 2024
944599f
Apply suggestions from code review
yadij Jul 1, 2024
3605a00
Upgrade Http::Stream::DeferredParams::reply as well
yadij Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/client_side.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
60 changes: 26 additions & 34 deletions src/http/Stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -34,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()
Expand Down Expand Up @@ -136,7 +134,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) {
Expand Down Expand Up @@ -264,11 +262,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;
yadij marked this conversation as resolved.
Show resolved Hide resolved

if (http->request->range)
buildRangeHeader();

const auto mb = reply->pack();

// dump now, so we do not output any body.
debugs(11, 2, "HTTP Client " << clientConnection);
Expand Down Expand Up @@ -377,7 +379,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's replace this unchecked/unwanted pointer with a reference instead of just changing its type:

Suggested change
clientIfRangeMatch(ClientHttpRequest * http, const HttpReplyPointer &reply)
clientIfRangeMatch(ClientHttpRequest * http, const HttpReply &reply)

... and adjust the function caller and pointer user accordingly.

The above will not make existing diff larger (because all these lines have to be changed anyway) but will improve code.

{
const TimeOrTag spec = http->request->header.getTimeOrTag(Http::HdrType::IF_RANGE);

Expand All @@ -387,7 +389,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible:

Suggested change
ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG);
const auto rep_tag = reply.header.getETag(Http::HdrType::ETAG);

Otherwise:

Suggested change
ETag rep_tag = reply->header.getETag(Http::HdrType::ETAG);
auto rep_tag = reply.header.getETag(Http::HdrType::ETAG);

The above suggestions account for another change request that replaces an unchecked pointer with a reference (but its essence is unrelated to that change request).

debugs(33, 3, "ETags: " << spec.tag.str << " and " <<
(rep_tag.str ? rep_tag.str : "<none>"));

Expand All @@ -414,42 +416,40 @@ 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()
{
rousskov marked this conversation as resolved.
Show resolved Hide resolved
HttpHeader *hdr = rep ? &rep->header : nullptr;
Assure(reply);
const 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,
* and client_side_reply determines hits candidates
*/
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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the only canonize() caller that supplies a pointer, please upgrade the corresponding canonize() method to take a reference instead of a pointer. The method already asserts that the pointer is not nil.

Suggested change
else if (!http->request->range->canonize(reply.getRaw()))
else if (!http->request->range->canonize(*reply))

range_err = "canonization failed";
else if (http->request->range->isComplex())
range_err = "too complex range header";
Expand All @@ -466,21 +466,21 @@ 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();

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 */
Expand Down Expand Up @@ -553,24 +553,16 @@ 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;
}

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.
Expand Down
13 changes: 5 additions & 8 deletions src/http/Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -127,13 +124,13 @@ 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

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
Expand All @@ -152,15 +149,15 @@ class Stream : public RefCountable

public:
clientStreamNode *node;
HttpReply *rep;
HttpReplyPointer reply;
StoreIOBuffer queuedBuffer;
};

DeferredParams deferredparams;
int64_t writtenToSocket;

private:
void prepareReply(HttpReply *);
void buildRangeHeader();
void packChunk(const StoreIOBuffer &bodyData, MemBuf &);
void packRange(StoreIOBuffer const &, MemBuf *);
void doClose();
Expand Down
6 changes: 3 additions & 3 deletions src/tests/stub_libhttp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,16 @@ int64_t Stream::getNextRangeOffset() const STUB_RETVAL(-1)
bool Stream::canPackMoreRanges() const STUB_RETVAL(false)
size_t Stream::lengthToSend(Range<int64_t> 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)
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
}