Skip to content

TS-4694: Some refactoring after SPDY is removed #825

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
masaori335 merged 1 commit into from
Jul 26, 2016
Merged

TS-4694: Some refactoring after SPDY is removed #825

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
1 change: 1 addition & 0 deletions proxy/ProxyClientSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ class ProxyClientSession : public VConnection
cancel_inactivity_timeout()
{
}
virtual const char *get_protocol_string() const = 0;

protected:
// XXX Consider using a bitwise flags variable for the following flags, so that we can make the best
Expand Down
16 changes: 0 additions & 16 deletions proxy/ProxyClientTransaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,22 +52,6 @@ ProxyClientTransaction::new_transaction()
DebugHttpTxn("[%" PRId64 "] Starting transaction %d using sm [%" PRId64 "]", parent->connection_id(),
parent->get_transact_count(), current_reader->sm_id);

// This is a temporary hack until we get rid of SPDY and can use virutal methods entirely
// to track protocol.
PluginIdentity *pi = dynamic_cast<PluginIdentity *>(this->get_netvc());
if (pi) {
current_reader->plugin_tag = pi->getPluginTag();
current_reader->plugin_id = pi->getPluginId();
} else {
const char *protocol_str = this->get_protocol_string();
// We don't set the plugin_tag for http, though in future we should probably log http as protocol
if (strlen(protocol_str) != 4 || strncmp("http", protocol_str, 4)) {
current_reader->plugin_tag = protocol_str;
// Since there is no more plugin, there is no plugin id for http/2
// We are copying along the plugin_tag as a standin for protocol name for logging
// and to detect a case in HttpTransaction (TS-3954)
}
}
current_reader->attach_client_session(this, sm_reader);
}

Expand Down
6 changes: 5 additions & 1 deletion proxy/ProxyClientTransaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection

virtual bool allow_half_open() const = 0;

virtual const char *get_protocol_string() const = 0;
virtual const char *
Copy link
Copy Markdown
Contributor

@zwoop zwoop Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to have these getter's take an (optional) argument to get the length as well? I noticed that later, in e.g. logging, we have to do strlen() on these static strings every time we use, which seems a bit wasteful, no? Maybe something like

get_protocol_string(size_t *len = NULL);

?

Copy link
Copy Markdown
Member

@shinrich shinrich Jul 25, 2016

Choose a reason for hiding this comment

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

I don't think adding a length argument makes much difference one way or the other. I guess it would reduce the logging call site to once call. But we are storing the protocol as a NULL terminated string, so we'd be doing the strlen in any case.

Copy link
Copy Markdown
Contributor

@zwoop zwoop Jul 25, 2016

Choose a reason for hiding this comment

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

I'm ok without it, but the way logging works, you will do strlen() on that string twice every time you log it (once to calculate the buffer space needed, and once to write the string).

Copy link
Copy Markdown
Contributor Author

@masaori335 masaori335 Jul 26, 2016

Choose a reason for hiding this comment

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

Yeah, it's better to reduce strlen calls. I start thinking about returning int or enum from here (something like TS_SSN_PROTOCOL_INDEX_HTTP_x_x) and convert it to string in logging phase.
Do we really need to return string at here?

Copy link
Copy Markdown
Contributor

@zwoop zwoop Jul 26, 2016

Choose a reason for hiding this comment

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

Can plugins set the string? If so, an enum would not suffice, right?

Copy link
Copy Markdown
Contributor Author

@masaori335 masaori335 Jul 26, 2016

Choose a reason for hiding this comment

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

Currently plugins can not set client_protocol of HttpSM ( which is going to be added by this patch ), but it could be set in the future. And it also has consistency for other stats in HttpSM (e.g. client_sec_protocol, client_cipher_suite). So, let's leave it string.

I'll file a new tickets for optimization.

get_protocol_string()
{
return parent ? parent->get_protocol_string() : NULL;
}

void
set_restart_immediate(bool val)
Expand Down
6 changes: 6 additions & 0 deletions proxy/http/Http1ClientSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ class Http1ClientSession : public ProxyClientSession
client_vc->cancel_inactivity_timeout();
}

virtual const char *
get_protocol_string() const
{
return "http";
}

private:
Http1ClientSession(Http1ClientSession &);

Expand Down
5 changes: 0 additions & 5 deletions proxy/http/Http1ClientTransaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,6 @@ class Http1ClientTransaction : public ProxyClientTransaction
{
return true;
}
virtual const char *
get_protocol_string() const
{
return "http";
}

void set_parent(ProxyClientSession *new_parent);

Expand Down
3 changes: 3 additions & 0 deletions proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ HttpSM::HttpSM()
client_tcp_reused(false),
client_ssl_reused(false),
client_connection_is_ssl(false),
client_protocol("-"),
client_sec_protocol("-"),
client_cipher_suite("-"),
server_transact_count(0),
Expand Down Expand Up @@ -536,6 +537,8 @@ HttpSM::attach_client_session(ProxyClientTransaction *client_vc, IOBufferReader
const char *cipher = ssl_vc->getSSLCipherSuite();
client_cipher_suite = cipher ? cipher : "-";
}
const char *protocol_str = client_vc->get_protocol_string();
client_protocol = protocol_str ? protocol_str : "-";

ink_release_assert(ua_session->get_half_close_flag() == false);
mutex = client_vc->mutex;
Expand Down
1 change: 1 addition & 0 deletions proxy/http/HttpSM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,7 @@ class HttpSM : public Continuation
// Info about client's SSL connection.
bool client_ssl_reused;
bool client_connection_is_ssl;
const char *client_protocol;
const char *client_sec_protocol;
const char *client_cipher_suite;
int server_transact_count;
Expand Down
12 changes: 0 additions & 12 deletions proxy/http2/Http2ClientSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,15 +434,3 @@ Http2ClientSession::state_complete_frame_read(int event, void *edata)
vio->reenable();
return 0;
}

int64_t
Http2ClientSession::getPluginId() const
{
return con_id;
}

char const *
Http2ClientSession::getPluginTag() const
{
return "http/2";
}
11 changes: 7 additions & 4 deletions proxy/http2/Http2ClientSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class Http2Frame
IOBufferReader *ioreader;
};

class Http2ClientSession : public ProxyClientSession, public PluginIdentity
class Http2ClientSession : public ProxyClientSession
{
public:
typedef ProxyClientSession super; ///< Parent type.
Expand Down Expand Up @@ -197,9 +197,6 @@ class Http2ClientSession : public ProxyClientSession, public PluginIdentity
return upgrade_context;
}

virtual char const *getPluginTag() const;
virtual int64_t getPluginId() const;

virtual int
get_transact_count() const
{
Expand All @@ -222,6 +219,12 @@ class Http2ClientSession : public ProxyClientSession, public PluginIdentity
return dying_event;
}

virtual const char *
get_protocol_string() const
{
return "http/2";
}

private:
Http2ClientSession(Http2ClientSession &); // noncopyable
Http2ClientSession &operator=(const Http2ClientSession &); // noncopyable
Expand Down
6 changes: 0 additions & 6 deletions proxy/http2/Http2Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,6 @@ class Http2Stream : public ProxyClientTransaction
return false;
}

virtual const char *
get_protocol_string() const
{
return "http/2";
}

virtual void set_active_timeout(ink_hrtime timeout_in);
virtual void set_inactivity_timeout(ink_hrtime timeout_in);
virtual void cancel_inactivity_timeout();
Expand Down
22 changes: 10 additions & 12 deletions proxy/logging/LogAccessHttp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,31 +619,29 @@ LogAccessHttp::marshal_client_req_http_version(char *buf)
int
LogAccessHttp::marshal_client_req_protocol_version(char *buf)
{
int len = INK_MIN_ALIGN;
char const *tag = m_http_sm->plugin_tag;
const char *protocol_str = m_http_sm->client_protocol;
int len = LogAccess::strlen(protocol_str);

if (!tag) {
// Set major & minor versions when protocol_str is not "http/2".
if (::strlen(protocol_str) == 4 && strncmp("http", protocol_str, 4) == 0) {
if (m_client_request) {
HTTPVersion versionObject = m_client_request->version_get();
int64_t major = HTTP_MAJOR(versionObject.m_version);
int64_t minor = HTTP_MINOR(versionObject.m_version);
if (major == 1 && minor == 1) {
tag = "http/1.1";
protocol_str = "http/1.1";
} else if (major == 1 && minor == 0) {
tag = "http/1.0";
} else if (major == 0 && minor == 9) {
tag = "http/0.9";
protocol_str = "http/1.0";
} // else invalid http version
len = LogAccess::strlen(tag);
} else {
tag = "*";
protocol_str = "*";
}
} else {
len = LogAccess::strlen(tag);

len = LogAccess::strlen(protocol_str);
}

if (buf) {
marshal_str(buf, tag, len);
marshal_str(buf, protocol_str, len);
}

return len;
Expand Down