Skip to content

slice: clean up 502 header/body generation. #6919

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

Merged
traeak merged 1 commit into from
Jul 20, 2020
Merged

slice: clean up 502 header/body generation. #6919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 3 additions & 6 deletions plugins/experimental/slice/Data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ struct Data {

sockaddr_storage m_client_ip;

// transaction pointer
TSHttpTxn m_txnp{nullptr};

// for pristine/effective url coming in
TSMBuffer m_urlbuf{nullptr};
TSMLoc m_urlloc{nullptr};
Expand Down Expand Up @@ -96,16 +99,10 @@ struct Data {
m_hostname[0] = '\0';
m_etag[0] = '\0';
m_lastmodified[0] = '\0';
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::DataCreate, 1);
#endif
}

~Data()
{
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::DataDestroy, 1);
#endif
if (nullptr != m_urlbuf) {
if (nullptr != m_urlloc) {
TSHandleMLocRelease(m_urlbuf, TS_NULL_MLOC, m_urlloc);
Expand Down
84 changes: 20 additions & 64 deletions plugins/experimental/slice/HttpHeader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,77 +290,33 @@ HttpHeader::toString() const
{
std::string res;

if (!isValid()) {
return "<null>";
}

TSHttpType const htype(type());

switch (htype) {
case TS_HTTP_TYPE_REQUEST: {
res.append(method());
if (isValid()) {
TSIOBuffer const iobufp = TSIOBufferCreate();
TSHttpHdrPrint(m_buffer, m_lochdr, iobufp);
TSIOBufferReader const reader = TSIOBufferReaderAlloc(iobufp);

if (nullptr != reader) {
TSIOBufferBlock block = TSIOBufferReaderStart(reader);
bool done = false;
while (!done && nullptr != block) {
int64_t avail = 0;
char const *blockptr = TSIOBufferBlockReadStart(block, reader, &avail);
if (0 < avail) {
res.append(blockptr, avail);
}
block = TSIOBufferBlockNext(block);
}

int urllen = 0;
char *const urlstr = urlString(&urllen);
if (nullptr != urlstr) {
res.append(" ");
res.append(urlstr, urllen);
TSfree(urlstr);
} else {
res.append(" UnknownURL");
TSIOBufferReaderFree(reader);
}

res.append(" HTTP/unparsed");
} break;

case TS_HTTP_TYPE_RESPONSE: {
char bufstr[1024];
/*
int const version = TSHttpHdrVersionGet(m_buffer, m_lochdr);
snprintf(bufstr, 1023, "%d ", version);
res.append(bufstr);
*/
res.append("HTTP/unparsed");

int const status = TSHttpHdrStatusGet(m_buffer, m_lochdr);
snprintf(bufstr, 1023, " %d ", status);
res.append(bufstr);

int reasonlen = 0;
char const *const hreason = reason(&reasonlen);

res.append(hreason, reasonlen);
} break;

default:
case TS_HTTP_TYPE_UNKNOWN:
res.append("UNKNOWN");
break;
TSIOBufferDestroy(iobufp);
}

res.append("\r\n");

int const numhdrs = TSMimeHdrFieldsCount(m_buffer, m_lochdr);

for (int indexhdr = 0; indexhdr < numhdrs; ++indexhdr) {
TSMLoc const locfield = TSMimeHdrFieldGet(m_buffer, m_lochdr, indexhdr);

int keylen = 0;
char const *const keystr = TSMimeHdrFieldNameGet(m_buffer, m_lochdr, locfield, &keylen);

res.append(keystr, keylen);
res.append(": ");
int vallen = 0;
char const *const valstr = TSMimeHdrFieldValueStringGet(m_buffer, m_lochdr, locfield, -1, &vallen);

res.append(valstr, vallen);
res.append("\r\n");

TSHandleMLocRelease(m_buffer, m_lochdr, locfield);
if (res.empty()) {
res = "<null>";
}

res.append("\r\n");

return res;
}

Expand Down
9 changes: 0 additions & 9 deletions plugins/experimental/slice/Stage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ struct Channel {
{
if (nullptr != m_reader) {
TSIOBufferReaderFree(m_reader);
#if defined(COLLECT_STATS)
TSStatIntDecrement(stats::Reader, 1);
#endif
}
if (nullptr != m_iobuf) {
TSIOBufferDestroy(m_iobuf);
Expand Down Expand Up @@ -65,9 +62,6 @@ struct Channel {
if (nullptr == m_iobuf) {
m_iobuf = TSIOBufferCreate();
m_reader = TSIOBufferReaderAlloc(m_iobuf);
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::Reader, 1);
#endif
} else {
int64_t const drained = drainReader();
if (0 < drained) {
Expand All @@ -85,9 +79,6 @@ struct Channel {
if (nullptr == m_iobuf) {
m_iobuf = TSIOBufferCreate();
m_reader = TSIOBufferReaderAlloc(m_iobuf);
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::Reader, 1);
#endif
} else {
int64_t const drained = drainReader();
if (0 < drained) {
Expand Down
8 changes: 0 additions & 8 deletions plugins/experimental/slice/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@
bool
handle_client_req(TSCont contp, TSEvent event, Data *const data)
{
#if defined(COLLECT_STATS)
stats::StatsRAI const rai(stats::ClientTime);
#endif
switch (event) {
case TS_EVENT_VCONN_READ_READY:
case TS_EVENT_VCONN_READ_COMPLETE: {
Expand Down Expand Up @@ -126,11 +123,6 @@ handle_client_req(TSCont contp, TSEvent event, Data *const data)
void
handle_client_resp(TSCont contp, TSEvent event, Data *const data)
{
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::Client, 1);
stats::StatsRAI const rai(stats::ClientTime);
#endif

switch (event) {
case TS_EVENT_VCONN_WRITE_READY: {
switch (data->m_blockstate) {
Expand Down
12 changes: 8 additions & 4 deletions plugins/experimental/slice/response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ bodyString416()
}

// Form a 502 response, preliminary
std::string const &
string502()
std::string
string502(int const httpver)
{
static std::string msg;
static std::mutex mutex;
Expand All @@ -68,10 +68,13 @@ string502()
bodystr.append("</body>\n");
bodystr.append("</html>\n");

char hverstr[64];
int const hlen =
snprintf(hverstr, sizeof(hverstr), "HTTP/%d.%d 502 Bad Gateway\r\n", TS_HTTP_MAJOR(httpver), TS_HTTP_MINOR(httpver));
msg.append(hverstr, hlen);

Copy link
Copy Markdown
Member

@shinrich shinrich Jul 15, 2020

Choose a reason for hiding this comment

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

I assume this is the heart of your 502 fixes for this PR. Much of the rest looks like more general cleanup.

Copy link
Copy Markdown
Contributor Author

@traeak traeak Jul 16, 2020

Choose a reason for hiding this comment

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

Yes sort of. The original PR was much more aggressive about trying to use the state machine but attempts failed and this cleanup was what was left over.

char clenstr[1024];
int const clen = snprintf(clenstr, sizeof(clenstr), "%lu", bodystr.size());

msg.append("HTTP/1.1 502 Bad Gateway\r\n");
msg.append("Content-Length: ");
msg.append(clenstr, clen);
msg.append("\r\n");
Expand All @@ -87,6 +90,7 @@ void
form416HeaderAndBody(HttpHeader &header, int64_t const contentlen, std::string const &bodystr)
{
header.removeKey(TS_MIME_FIELD_LAST_MODIFIED, TS_MIME_LEN_LAST_MODIFIED);
header.removeKey(TS_MIME_FIELD_EXPIRES, TS_MIME_LEN_EXPIRES);
header.removeKey(TS_MIME_FIELD_ETAG, TS_MIME_LEN_ETAG);
header.removeKey(TS_MIME_FIELD_ACCEPT_RANGES, TS_MIME_LEN_ACCEPT_RANGES);

Expand Down
2 changes: 1 addition & 1 deletion plugins/experimental/slice/response.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include "HttpHeader.h"
#include <string>

std::string const &string502();
std::string string502(int const httpver);

std::string const &bodyString416();

Expand Down
23 changes: 4 additions & 19 deletions plugins/experimental/slice/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,6 @@ enum HeaderState {
HeaderState
handleFirstServerHeader(Data *const data, TSCont const contp)
{
#if defined(COLLECT_STATS)
stats::StatsRAI const rai(stats::FirstHeaderTime);
#endif

HeaderState state = HeaderState::Good;

HttpHeader header(data->m_resp_hdrmgr.m_buffer, data->m_resp_hdrmgr.m_lochdr);
Expand All @@ -116,7 +112,7 @@ handleFirstServerHeader(Data *const data, TSCont const contp)
// Should run TSVIONSetBytes(output_io, hlen + bodybytes);
int64_t const hlen = TSHttpHdrLengthGet(header.m_buffer, header.m_lochdr);
int64_t const clen = contentLengthFrom(header);
DEBUG_LOG("Passthru: header: %" PRId64 " body: %" PRId64, hlen, clen);
DEBUG_LOG("Passthru bytes: header: %" PRId64 " body: %" PRId64, hlen, clen);
if (clen != INT64_MAX) {
TSVIONBytesSet(output_vio, hlen + clen);
} else {
Expand All @@ -129,9 +125,9 @@ handleFirstServerHeader(Data *const data, TSCont const contp)

ContentRange const blockcr = contentRangeFrom(header);

// 206 with bad content range?
// 206 with bad content range -- should NEVER happen.
if (!blockcr.isValid()) {
static std::string const &msg502 = string502();
std::string const msg502 = string502(header.version());
TSVIONBytesSet(output_vio, msg502.size());
TSIOBufferWrite(output_buf, msg502.data(), msg502.size());
TSVIOReenable(output_vio);
Expand Down Expand Up @@ -343,10 +339,6 @@ logSliceError(char const *const message, Data const *const data, HttpHeader cons
bool
handleNextServerHeader(Data *const data, TSCont const contp)
{
#if defined(COLLECT_STATS)
stats::StatsRAI const rai(stats::NextHeaderTime);
#endif

// block response header
HttpHeader header(data->m_resp_hdrmgr.m_buffer, data->m_resp_hdrmgr.m_lochdr);
if (TSIsDebugTagSet(PLUGIN_NAME)) {
Expand Down Expand Up @@ -490,11 +482,6 @@ handleNextServerHeader(Data *const data, TSCont const contp)
void
handle_server_resp(TSCont contp, TSEvent event, Data *const data)
{
#if defined(COLLECT_STATS)
TSStatIntIncrement(stats::Server, 1);
stats::StatsRAI const rai(stats::ServerTime);
#endif

switch (event) {
case TS_EVENT_VCONN_READ_READY: {
if (data->m_blockstate == BlockState::Passthru) {
Expand All @@ -512,11 +499,11 @@ handle_server_resp(TSCont contp, TSEvent event, Data *const data)
TSVIONDoneSet(input_vio, TSVIONDoneGet(input_vio) + consumed);

// the server response header didn't fit into the input buffer.
// wait for more data from upstream
if (TS_PARSE_CONT == res) {
return;
}

// very first server response header
bool headerStat = false;

if (TS_PARSE_DONE == res) {
Expand Down Expand Up @@ -545,8 +532,6 @@ handle_server_resp(TSCont contp, TSEvent event, Data *const data)
}

data->m_server_block_header_parsed = true;
} else if (TS_PARSE_CONT == res) {
return;
}

// kill the upstream and allow dnstream to clean up
Expand Down
Loading