You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by am...@apache.org on 2014/09/19 15:30:22 UTC
[1/2] git commit: TS-2802: Additional fixups. Changed NetVCOptions to
use ats_scoped_str instead of a raw pointer. Added comments.
Repository: trafficserver
Updated Branches:
refs/heads/master a9905459d -> 4a143e584
TS-2802: Additional fixups.
Changed NetVCOptions to use ats_scoped_str instead of a raw pointer.
Added comments.
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/4b48b2c2
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/4b48b2c2
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/4b48b2c2
Branch: refs/heads/master
Commit: 4b48b2c208f7e5607c27653bb407784e2cac1926
Parents: a990545
Author: Alan M. Carroll <am...@network-geographics.com>
Authored: Thu Sep 18 15:45:26 2014 -0500
Committer: Alan M. Carroll <am...@network-geographics.com>
Committed: Thu Sep 18 15:45:26 2014 -0500
----------------------------------------------------------------------
iocore/net/I_NetVConnection.h | 30 ++++++++++++++++++------------
iocore/net/P_UnixNetVConnection.h | 1 -
iocore/net/SSLNetVConnection.cc | 4 ++--
3 files changed, 20 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4b48b2c2/iocore/net/I_NetVConnection.h
----------------------------------------------------------------------
diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
index dcf86a7..cf97c64 100644
--- a/iocore/net/I_NetVConnection.h
+++ b/iocore/net/I_NetVConnection.h
@@ -159,7 +159,9 @@ struct NetVCOptions {
EventType etype;
- char * sni_servername; // SSL SNI to origin
+ /** Server name to use for SNI data on an outbound connection.
+ */
+ ats_scoped_str sni_servername;
/// Reset all values to defaults.
void reset();
@@ -167,31 +169,35 @@ struct NetVCOptions {
void set_sock_param(int _recv_bufsize, int _send_bufsize, unsigned long _opt_flags,
unsigned long _packet_mark = 0, unsigned long _packet_tos = 0);
- NetVCOptions() : sni_servername(NULL) {
+ NetVCOptions() {
reset();
}
~NetVCOptions() {
- ats_free(sni_servername);
}
- void set_sni_servername(const char * name, size_t len) {
+ /** Set the SNI server name.
+ A local copy is made of @a name.
+ */
+ self& set_sni_servername(const char * name, size_t len) {
IpEndpoint ip;
- ats_free(sni_servername);
- sni_servername = NULL;
// Literal IPv4 and IPv6 addresses are not permitted in "HostName".(rfc6066#section-3)
if (ats_ip_pton(ts::ConstBuffer(name, len), &ip) != 0) {
sni_servername = ats_strndup(name, len);
+ } else {
+ sni_servername = NULL;
}
+ return *this;
}
- NetVCOptions & operator=(const NetVCOptions & opt) {
- if (&opt != this) {
- ats_free(this->sni_servername);
- memcpy(this, &opt, sizeof(opt));
- if (opt.sni_servername) {
- this->sni_servername = ats_strdup(opt.sni_servername);
+ self& operator=(self const& that) {
+ if (&that != this) {
+ sni_servername = NULL; // release any current name.
+ memcpy(this, &that, sizeof(self));
+ if (that.sni_servername) {
+ sni_servername.release(); // otherwise we'll free the source string.
+ this->sni_servername = ats_strdup(that.sni_servername);
}
}
return *this;
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4b48b2c2/iocore/net/P_UnixNetVConnection.h
----------------------------------------------------------------------
diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h
index 0b7845e..7fb597d 100644
--- a/iocore/net/P_UnixNetVConnection.h
+++ b/iocore/net/P_UnixNetVConnection.h
@@ -66,7 +66,6 @@ NetVCOptions::reset()
etype = ET_NET;
- ats_free(sni_servername);
sni_servername = NULL;
}
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4b48b2c2/iocore/net/SSLNetVConnection.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
index d3aa858..7978013 100644
--- a/iocore/net/SSLNetVConnection.cc
+++ b/iocore/net/SSLNetVConnection.cc
@@ -666,9 +666,9 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
#if TS_USE_TLS_SNI
if (options.sni_servername) {
if (SSL_set_tlsext_host_name(ssl, options.sni_servername)) {
- Debug("ssl", "using SNI name '%s' for client handshake", options.sni_servername);
+ Debug("ssl", "using SNI name '%s' for client handshake", options.sni_servername.get());
} else {
- Debug("ssl.error","failed to set SNI name '%s' for client handshake", options.sni_servername);
+ Debug("ssl.error","failed to set SNI name '%s' for client handshake", options.sni_servername.get());
SSL_INCREMENT_DYN_STAT(ssl_sni_name_set_failure);
}
}
Re: [1/2] git commit: TS-2802: Additional fixups. Changed NetVCOptions
to use ats_scoped_str instead of a raw pointer. Added comments.
Posted by James Peach <jp...@apache.org>.
On Sep 19, 2014, at 8:38 AM, Alan M. Carroll <am...@network-geographics.com> wrote:
> Friday, September 19, 2014, 10:12:29 AM, you wrote:
>
>
>>> + sni_servername.release(); // otherwise we'll free the source string.
>
>
>> Why don't you want to free the source string in this case?
>
> Because it's still in the source object and therefore it would be freed again at some point in the future.
Oh, because of the memcpy ... yeh
Re: [1/2] git commit: TS-2802: Additional fixups. Changed NetVCOptions to use
ats_scoped_str instead of a raw pointer. Added comments.
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
Friday, September 19, 2014, 10:12:29 AM, you wrote:
>> + sni_servername.release(); // otherwise we'll free the source string.
> Why don't you want to free the source string in this case?
Because it's still in the source object and therefore it would be freed again at some point in the future.
Re: [1/2] git commit: TS-2802: Additional fixups. Changed NetVCOptions
to use ats_scoped_str instead of a raw pointer. Added comments.
Posted by James Peach <jp...@apache.org>.
On Sep 19, 2014, at 6:30 AM, amc@apache.org wrote:
> Repository: trafficserver
> Updated Branches:
> refs/heads/master a9905459d -> 4a143e584
>
>
> TS-2802: Additional fixups.
> Changed NetVCOptions to use ats_scoped_str instead of a raw pointer.
> Added comments.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/4b48b2c2
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/4b48b2c2
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/4b48b2c2
>
> Branch: refs/heads/master
> Commit: 4b48b2c208f7e5607c27653bb407784e2cac1926
> Parents: a990545
> Author: Alan M. Carroll <am...@network-geographics.com>
> Authored: Thu Sep 18 15:45:26 2014 -0500
> Committer: Alan M. Carroll <am...@network-geographics.com>
> Committed: Thu Sep 18 15:45:26 2014 -0500
>
> ----------------------------------------------------------------------
> iocore/net/I_NetVConnection.h | 30 ++++++++++++++++++------------
> iocore/net/P_UnixNetVConnection.h | 1 -
> iocore/net/SSLNetVConnection.cc | 4 ++--
> 3 files changed, 20 insertions(+), 15 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4b48b2c2/iocore/net/I_NetVConnection.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
> index dcf86a7..cf97c64 100644
> --- a/iocore/net/I_NetVConnection.h
> +++ b/iocore/net/I_NetVConnection.h
> @@ -159,7 +159,9 @@ struct NetVCOptions {
>
> EventType etype;
>
> - char * sni_servername; // SSL SNI to origin
> + /** Server name to use for SNI data on an outbound connection.
> + */
> + ats_scoped_str sni_servername;
>
> /// Reset all values to defaults.
> void reset();
> @@ -167,31 +169,35 @@ struct NetVCOptions {
> void set_sock_param(int _recv_bufsize, int _send_bufsize, unsigned long _opt_flags,
> unsigned long _packet_mark = 0, unsigned long _packet_tos = 0);
>
> - NetVCOptions() : sni_servername(NULL) {
> + NetVCOptions() {
> reset();
> }
>
> ~NetVCOptions() {
> - ats_free(sni_servername);
> }
>
> - void set_sni_servername(const char * name, size_t len) {
> + /** Set the SNI server name.
> + A local copy is made of @a name.
> + */
> + self& set_sni_servername(const char * name, size_t len) {
> IpEndpoint ip;
>
> - ats_free(sni_servername);
> - sni_servername = NULL;
> // Literal IPv4 and IPv6 addresses are not permitted in "HostName".(rfc6066#section-3)
> if (ats_ip_pton(ts::ConstBuffer(name, len), &ip) != 0) {
> sni_servername = ats_strndup(name, len);
> + } else {
> + sni_servername = NULL;
> }
> + return *this;
> }
>
> - NetVCOptions & operator=(const NetVCOptions & opt) {
> - if (&opt != this) {
> - ats_free(this->sni_servername);
> - memcpy(this, &opt, sizeof(opt));
> - if (opt.sni_servername) {
> - this->sni_servername = ats_strdup(opt.sni_servername);
> + self& operator=(self const& that) {
> + if (&that != this) {
> + sni_servername = NULL; // release any current name.
> + memcpy(this, &that, sizeof(self));
> + if (that.sni_servername) {
> + sni_servername.release(); // otherwise we'll free the source string.
Why don't you want to free the source string in this case?
[2/2] git commit: TS-3087: Fix flow control stall on low water marks.
Posted by am...@apache.org.
TS-3087: Fix flow control stall on low water marks.
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/4a143e58
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/4a143e58
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/4a143e58
Branch: refs/heads/master
Commit: 4a143e584a000d0d591ef323e0789ae80de34b1a
Parents: 4b48b2c
Author: Alan M. Carroll <am...@network-geographics.com>
Authored: Fri Sep 19 08:28:57 2014 -0500
Committer: Alan M. Carroll <am...@network-geographics.com>
Committed: Fri Sep 19 08:28:57 2014 -0500
----------------------------------------------------------------------
iocore/net/I_NetVConnection.h | 22 +++++++++++++++++++++-
iocore/net/P_UnixNetVConnection.h | 2 ++
iocore/net/UnixNetVConnection.cc | 12 ++++++++++++
proxy/http/HttpTunnel.cc | 9 +++++++++
4 files changed, 44 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4a143e58/iocore/net/I_NetVConnection.h
----------------------------------------------------------------------
diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
index cf97c64..0927144 100644
--- a/iocore/net/I_NetVConnection.h
+++ b/iocore/net/I_NetVConnection.h
@@ -434,6 +434,17 @@ public:
/** @return current inactivity_timeout value in nanosecs */
virtual ink_hrtime get_inactivity_timeout() = 0;
+ /** Force an @a event if a write operation empties the write buffer.
+
+ This event will be sent to the VIO, the same place as other IO events.
+ Use an @a event value of 0 to cancel the trap.
+
+ The event is sent only the next time the write buffer is emptied, not
+ every future time. The event is sent only if otherwise no event would
+ be generated.
+ */
+ virtual void trapWriteBufferEmpty(int event = VC_EVENT_WRITE_READY);
+
/** Returns local sockaddr storage. */
sockaddr const* get_local_addr();
@@ -535,6 +546,8 @@ protected:
bool is_internal_request;
/// Set if this connection is transparent.
bool is_transparent;
+ /// Set if the next write IO that empties the write buffer should generate an event.
+ int write_buffer_empty_event;
};
inline
@@ -545,10 +558,17 @@ NetVConnection::NetVConnection():
got_local_addr(0),
got_remote_addr(0),
is_internal_request(false),
- is_transparent(false)
+ is_transparent(false),
+ write_buffer_empty_event(0)
{
ink_zero(local_addr);
ink_zero(remote_addr);
}
+inline void
+NetVConnection::trapWriteBufferEmpty(int event)
+{
+ write_buffer_empty_event = event;
+}
+
#endif
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4a143e58/iocore/net/P_UnixNetVConnection.h
----------------------------------------------------------------------
diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h
index 7fb597d..4690556 100644
--- a/iocore/net/P_UnixNetVConnection.h
+++ b/iocore/net/P_UnixNetVConnection.h
@@ -249,6 +249,8 @@ public:
virtual void set_remote_addr();
virtual int set_tcp_init_cwnd(int init_cwnd);
virtual void apply_options();
+
+ friend void write_to_net_io(NetHandler*, UnixNetVConnection*, EThread*);
};
extern ClassAllocator<UnixNetVConnection> netVCAllocator;
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4a143e58/iocore/net/UnixNetVConnection.cc
----------------------------------------------------------------------
diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc
index cf1d9c8..ec8605c 100644
--- a/iocore/net/UnixNetVConnection.cc
+++ b/iocore/net/UnixNetVConnection.cc
@@ -474,6 +474,8 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
write_signal_error(nh, vc, (int)-r);
return;
} else {
+ int wbe_event = vc->write_buffer_empty_event; // save so we can clear if needed.
+
NET_SUM_DYN_STAT(net_write_bytes_stat, r);
// Remove data from the buffer and signal continuation.
@@ -482,12 +484,22 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
ink_assert(buf.reader()->read_avail() >= 0);
s->vio.ndone += r;
+ // If the empty write buffer trap is set, clear it.
+ if (!(buf.reader()->is_read_avail_more_than(0)))
+ vc->write_buffer_empty_event = 0;
+
net_activity(vc, thread);
// If there are no more bytes to write, signal write complete,
ink_assert(ntodo >= 0);
if (s->vio.ntodo() <= 0) {
write_signal_done(VC_EVENT_WRITE_COMPLETE, nh, vc);
return;
+ } else if (signalled && (wbe_event != vc->write_buffer_empty_event)) {
+ // @a signalled means we won't send an event, and the event values differing means we
+ // had a write buffer trap and cleared it, so we need to send it now.
+ Debug("amc", "empty write buffer trap [%d]", wbe_event);
+ if (write_signal_and_update(wbe_event, vc) != EVENT_CONT)
+ return;
} else if (!signalled) {
if (write_signal_and_update(VC_EVENT_WRITE_READY, vc) != EVENT_CONT) {
return;
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4a143e58/proxy/http/HttpTunnel.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpTunnel.cc b/proxy/http/HttpTunnel.cc
index ef0da2c..d71f6c8 100644
--- a/proxy/http/HttpTunnel.cc
+++ b/proxy/http/HttpTunnel.cc
@@ -1241,6 +1241,15 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer* c)
srcp->read_vio->reenable();
// Kick source producer to get flow ... well, flowing.
this->producer_handler(VC_EVENT_READ_READY, srcp);
+ } else {
+ // We can stall for small thresholds on network sinks because this event happens
+ // before the actual socket write. So we trap for the buffer becoming empty to
+ // make sure we get an event to unthrottle after the write.
+ if (HT_HTTP_CLIENT == c->vc_type) {
+ NetVConnection* netvc = dynamic_cast<NetVConnection*>(c->write_vio->vc_server);
+ if (netvc) // really, this should always be true.
+ netvc->trapWriteBufferEmpty();
+ }
}
}
p->read_vio->reenable();
Re: [1/2] git commit: TS-2802: Additional fixups. Changed NetVCOptions
to use ats_scoped_str instead of a raw pointer. Added comments.
Posted by James Peach <jp...@apache.org>.
On Sep 19, 2014, at 6:30 AM, amc@apache.org wrote:
> Repository: trafficserver
> Updated Branches:
> refs/heads/master a9905459d -> 4a143e584
>
>
> TS-2802: Additional fixups.
> Changed NetVCOptions to use ats_scoped_str instead of a raw pointer.
> Added comments.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/4b48b2c2
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/4b48b2c2
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/4b48b2c2
>
> Branch: refs/heads/master
> Commit: 4b48b2c208f7e5607c27653bb407784e2cac1926
> Parents: a990545
> Author: Alan M. Carroll <am...@network-geographics.com>
> Authored: Thu Sep 18 15:45:26 2014 -0500
> Committer: Alan M. Carroll <am...@network-geographics.com>
> Committed: Thu Sep 18 15:45:26 2014 -0500
>
> ----------------------------------------------------------------------
> iocore/net/I_NetVConnection.h | 30 ++++++++++++++++++------------
> iocore/net/P_UnixNetVConnection.h | 1 -
> iocore/net/SSLNetVConnection.cc | 4 ++--
> 3 files changed, 20 insertions(+), 15 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4b48b2c2/iocore/net/I_NetVConnection.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
> index dcf86a7..cf97c64 100644
> --- a/iocore/net/I_NetVConnection.h
> +++ b/iocore/net/I_NetVConnection.h
> @@ -159,7 +159,9 @@ struct NetVCOptions {
>
> EventType etype;
>
> - char * sni_servername; // SSL SNI to origin
> + /** Server name to use for SNI data on an outbound connection.
> + */
> + ats_scoped_str sni_servername;
>
> /// Reset all values to defaults.
> void reset();
> @@ -167,31 +169,35 @@ struct NetVCOptions {
> void set_sock_param(int _recv_bufsize, int _send_bufsize, unsigned long _opt_flags,
> unsigned long _packet_mark = 0, unsigned long _packet_tos = 0);
>
> - NetVCOptions() : sni_servername(NULL) {
> + NetVCOptions() {
> reset();
> }
>
> ~NetVCOptions() {
> - ats_free(sni_servername);
> }
>
> - void set_sni_servername(const char * name, size_t len) {
> + /** Set the SNI server name.
> + A local copy is made of @a name.
> + */
> + self& set_sni_servername(const char * name, size_t len) {
> IpEndpoint ip;
>
> - ats_free(sni_servername);
> - sni_servername = NULL;
> // Literal IPv4 and IPv6 addresses are not permitted in "HostName".(rfc6066#section-3)
> if (ats_ip_pton(ts::ConstBuffer(name, len), &ip) != 0) {
> sni_servername = ats_strndup(name, len);
> + } else {
> + sni_servername = NULL;
> }
> + return *this;
> }
>
> - NetVCOptions & operator=(const NetVCOptions & opt) {
> - if (&opt != this) {
> - ats_free(this->sni_servername);
> - memcpy(this, &opt, sizeof(opt));
> - if (opt.sni_servername) {
> - this->sni_servername = ats_strdup(opt.sni_servername);
> + self& operator=(self const& that) {
> + if (&that != this) {
> + sni_servername = NULL; // release any current name.
> + memcpy(this, &that, sizeof(self));
> + if (that.sni_servername) {
> + sni_servername.release(); // otherwise we'll free the source string.
Why don't you want to free the source string in this case?