Add UMA for the kernel's TCP RTT estimate.

Add UMA for the kernel's TCP RTT estimate.

This change adds UMA to log the kernel's RTT estimate at the time a connection
is closed.

We already log UMA for TCP connection times (see Net.TCP_Connection_Latency).
This is useful, but may not capture true network RTT due to SYN retransmits.
Additionally, the linux kernel continues to update its RTT estimate over the
life of the TCP connection using an exponentially weighted moving average, so
this estimate should be affected less by outliers than a single RTT sample
from the TCP SYN.

My current open questions (to net maintainers):
* Is TCPClientSocket the right place to hook in for the purpose of recording
  round trip times?
* Which Chrome connections do/do not use TCPClientSocket? Do SSL sockets use
  TCPClientSocket internally? What about proxy connections? Any other
  interesting/important cases I should be aware of here?
* If TCPClientSocket is the right place to hook in, am I catching all cases
  where a TCPClientSocket can be closed? I hooked into the destructor and
  Disconnect() - are there other places where the socket can get closed
  that I should also be hooking?

Committed: https://crrev.com/8b62f477c62822a4419298f690ae06907544f516
Cr-Commit-Position: refs/heads/master@{#337827}

[Bryan McQuade, 2015-07-07 18:01:22]

bmcquade@chromium.org changed reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[Bryan McQuade, 2015-07-07 18:01:23]

bmcquade@chromium.org changed required reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[Bryan McQuade, 2015-07-07 18:02:26]

mmenke: PTAL for net code
asvitkine: PTAL for histograms.xml
jri, davidben: FYI

[Alexei Svitkine (slow), 2015-07-07 18:50:56]

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:358: if (previously_disconnected_) {
Nit: No {}'s for 1-line bodies.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:363:
UMA_HISTOGRAM_CUSTOM_TIMES("Net.TcpRttAtSocketClose", rtt,
Nit: Optional suggestion:

Rename to Net.TcpRtt.AtSocketClose.

That way, if you want to log this value at other intervals, it's easy to add
another suffix while keeping the names more readable.

(Change the xml name too if you make this change.)

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:365: base::TimeDelta::FromMinutes(10), 100);
Nit: Why do you need 100 buckets instead of the usual 50?

In fact, maybe the default UMA_HISTOGRAM_MEDIUM_TIMES() could work here? The
difference is it has a max of 3mins instead of 10. Do you care much about the
buckets between 3mins and 10mins?

[Bryan McQuade, 2015-07-07 19:35:14]

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:358: if (previously_disconnected_) {
On 2015/07/07 18:50:56, Alexei Svitkine wrote:
> Nit: No {}'s for 1-line bodies.

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:363:
UMA_HISTOGRAM_CUSTOM_TIMES("Net.TcpRttAtSocketClose", rtt,
On 2015/07/07 18:50:56, Alexei Svitkine wrote:
> Nit: Optional suggestion:
> 
> Rename to Net.TcpRtt.AtSocketClose.
> 
> That way, if you want to log this value at other intervals, it's easy to add
> another suffix while keeping the names more readable.
> 
> (Change the xml name too if you make this change.)

Great suggestion, thanks! Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:365: base::TimeDelta::FromMinutes(10), 100);
On 2015/07/07 18:50:56, Alexei Svitkine wrote:
> Nit: Why do you need 100 buckets instead of the usual 50?
> 
> In fact, maybe the default UMA_HISTOGRAM_MEDIUM_TIMES() could work here? The
> difference is it has a max of 3mins instead of 10. Do you care much about the
> buckets between 3mins and 10mins?

I chose this bucketing strategy to match the similar Net.TCP_Connection_Latency
histogram. We'll want to compare the histograms so I thought it best to use the
same bucketing strategy.

https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/transpo...

[Alexei Svitkine (slow), 2015-07-07 19:43:35]

lgtm

[mmenke, 2015-07-07 19:53:50]

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:184: EmitTCPMetricsHistogramsOnSocketClose();
Suggest a couple things:

1) Rename this to EmitTCPMetricsHistograms (The "socket" is implied, and it's
not clear what "close" means).
2) Move this to DoDisconnect.  We probably won't get anything in the other
disconnect case, but the Disconnect / DoDisconnect split makes me very nervous
about regressions.
3) Call Disconnection on destruction.  We'll be doing a little extra work, but
it looks pretty negligible to me.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:358: if (previously_disconnected_) {
Is this needed?  Seems weird to exclude the second value it a socket was
previously disconnected and then reused (Something I'd rather not allow, but we
do).

I think what you're concerned about is a socket being disconnected and then us
gathering measurements again in the destructor, but GetEstimatedRoundTripTime()
already returns false in that case (Since socket_->Close() will have been
called, which nulls out the...erm...socket_'s socket_).

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:362: if (socket_ &&
socket_->GetEstimatedRoundTripTime(&rtt)) {
socket_ can't be NULL (It's set to a non-NULL value on construction, and never
reset).

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.h:99: void EmitTCPMetricsHistogramsOnSocketClose();
Think this is worth a comment.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:725: &info, &info_len) == 0 &&
Can we just make this into a method?

It's a bit more code, and still has to be TCP_INFO guarded, but the getsocketopt
+ info_len check is sufficiently ugly that it may be a bit of an improvement.

Could just make it a static method in this file, and pass in the fd and the
tcp_info, and have it return false on failure.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:749: bool
TCPSocketLibevent::GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) {
How do you intend to use this to inform future code decisions?  Just wondering
what role localhost requests and the like play here, and if we care.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:751: if (!socket_ || !socket_->IsConnected())
{
The IsConnected() check seems concerning - if the server hangs up on us, it will
return false, but I think it may still have stats we can get?

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:753: }
nit:  Remove braces.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:763: *out_rtt =
base::TimeDelta::FromMicroseconds(info.tcpi_rtt);
Suggest getting rid of success variable, and just returning true here.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:764: }
nit:  Remove braces.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:765: #endif
nit:  #endif  // defined(TCP_INFO)

(Not done in most places in this file, but it's the preferred style).

[Bryan McQuade, 2015-07-07 21:03:33]

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:184: EmitTCPMetricsHistogramsOnSocketClose();
On 2015/07/07 19:53:49, mmenke wrote:
> Suggest a couple things:
> 
> 1) Rename this to EmitTCPMetricsHistograms (The "socket" is implied, and it's
> not clear what "close" means).

I changed to EmitTCPMetricsHistogramsOnDisconnect. I think it's important to
capture in the signature that the metric being recorded in the current
implementation only want to be recorded at disconnect time (the metric is named
Net.TcpRtt.AtDisconnect). Happy to remove the 'OnDisconnect' part of the method
name but I worry that it makes it easier for someone modifying this code in the
future to start calling the method at other locations, and inadvertently logging
Net.TcpRtt.AtDisconnect at places other than disconnection.

> 2) Move this to DoDisconnect.  We probably won't get anything in the other

> disconnect case, but the Disconnect / DoDisconnect split makes me very nervous
> about regressions.

Done. I noticed that DoDisconnect also gets called from
TCPClientSocket::DoConnectComplete with the comment " Close whatever partially
connected socket we currently have.". I wasn't sure if it was desirable to try
to emit histograms for such a socket. What do you think?

> 3) Call Disconnection on destruction.  We'll be doing a little extra work, but
> it looks pretty negligible to me.

Done. Note that Disconnect is virtual, so if TCPClientSocket is subclassed and
Disconnect overridden, the overridden implementation won't get called when the
derived class destructs (unless the derived class also calls Disconnect in its
destructor...). Not sure if this is a concern or not. I could also call the
non-virtual DoDisconnect in the destructor. Let me know which you prefer.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:358: if (previously_disconnected_) {
Removed.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.cc:362: if (socket_ &&
socket_->GetEstimatedRoundTripTime(&rtt)) {
On 2015/07/07 19:53:49, mmenke wrote:
> socket_ can't be NULL (It's set to a non-NULL value on construction, and never
> reset).

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:749: bool
TCPSocketLibevent::GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) {
I'm mostly interested in understanding TCP RTT in UMA, but the team working on
the network quality estimator is also interested in using this to improve their
estimation. I imagine they'd want to ignore connections to localhost.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:751: if (!socket_ || !socket_->IsConnected())
{
Yes, good point. I had added that to prevent logging the RTT when the kernel
doesn't have an estimate yet, but I think it's better for me to heck if the
kernel estimate is zero and discard if so. I suppose that technically a
connection could have a zero-microsecond RTT but I'm inclined to interpret zero
values as indicating the kernel doesn't yet have an estimate.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:753: }
On 2015/07/07 19:53:49, mmenke wrote:
> nit:  Remove braces.

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:763: *out_rtt =
base::TimeDelta::FromMicroseconds(info.tcpi_rtt);
On 2015/07/07 19:53:49, mmenke wrote:
> Suggest getting rid of success variable, and just returning true here.

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:764: }
On 2015/07/07 19:53:50, mmenke wrote:
> nit:  Remove braces.

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:765: #endif
On 2015/07/07 19:53:49, mmenke wrote:
> nit:  #endif  // defined(TCP_INFO)
> 
> (Not done in most places in this file, but it's the preferred style).

Done.

[Bryan McQuade, 2015-07-07 21:05:11]

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_client_s...
net/socket/tcp_client_socket.h:99: void EmitTCPMetricsHistogramsOnSocketClose();
On 2015/07/07 19:53:49, mmenke wrote:
> Think this is worth a comment.

Done.

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/1215543003/diff/60001/net/socket/tcp_socket_l...
net/socket/tcp_socket_libevent.cc:725: &info, &info_len) == 0 &&
On 2015/07/07 19:53:49, mmenke wrote:
> Can we just make this into a method?
> 
> It's a bit more code, and still has to be TCP_INFO guarded, but the
getsocketopt
> + info_len check is sufficiently ugly that it may be a bit of an improvement.
> 
> Could just make it a static method in this file, and pass in the fd and the
> tcp_info, and have it return false on failure.

Done.

[mmenke, 2015-07-07 21:27:29]

One other thing worth noting (Which you've probably already thought about): 
This will record one value per socket, regardless of what the socket has been
used for - if a socket has been used for 2000 HTTP requests, or 0 because it was
a preconnect, or 0 because it wasn't used for HTTP, etc.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:183: void TCPClientSocket::Disconnect() {
On 2015/07/07 21:03:32, Bryan McQuade wrote:
> On 2015/07/07 19:53:49, mmenke wrote:
> > Suggest a couple things:
> > 
> > 1) Rename this to EmitTCPMetricsHistograms (The "socket" is implied, and
it's
> > not clear what "close" means).
> 
> I changed to EmitTCPMetricsHistogramsOnDisconnect. I think it's important to
> capture in the signature that the metric being recorded in the current
> implementation only want to be recorded at disconnect time (the metric is
named
> Net.TcpRtt.AtDisconnect). Happy to remove the 'OnDisconnect' part of the
method
> name but I worry that it makes it easier for someone modifying this code in
the
> future to start calling the method at other locations, and inadvertently
logging
> Net.TcpRtt.AtDisconnect at places other than disconnection.
> 
> > 2) Move this to DoDisconnect.  We probably won't get anything in the other
> 
> > disconnect case, but the Disconnect / DoDisconnect split makes me very
nervous
> > about regressions.
> 
> Done. I noticed that DoDisconnect also gets called from
> TCPClientSocket::DoConnectComplete with the comment " Close whatever partially
> connected socket we currently have.". I wasn't sure if it was desirable to try
> to emit histograms for such a socket. What do you think?

This is a valid concern.  While I suspect we'll see an estimated RTT of 0 in
that case, I'm not confident of it.  One option would be to pass a bool into
DoDisconnect indicating if we're calling it due to a connection failure or not.

> 
> > 3) Call Disconnection on destruction.  We'll be doing a little extra work,
but
> > it looks pretty negligible to me.
> 
> Done. Note that Disconnect is virtual, so if TCPClientSocket is subclassed and
> Disconnect overridden, the overridden implementation won't get called when the
> derived class destructs (unless the derived class also calls Disconnect in its
> destructor...). Not sure if this is a concern or not. I could also call the
> non-virtual DoDisconnect in the destructor. Let me know which you prefer.

The class really isn't meant to be subclassed - it's only done for tests.  Some
sticklers would doubtless disagree disagree with me on this, but I think it's
fine as-is.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.cc:762: // When the kernel doesn't have an RTT
estimate, tcpi_rtt is zero.
May be worth mentioning that it may be 0 in other cases, but most of those are
probably localhost.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:67: // Gets the estimated RTT. Returns false if
the RTT is unavailable.
Maybe add something like "May also return false when estimated RTT is 0."

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:68: bool
GetEstimatedRoundTripTime(base::TimeDelta* out_rtt);
Can this be const?  (And the method that calls it).  Admittedly, calling methods
on socket descriptors can be const from a C++ perspective, but not logically
const in terms of mutating the state of a socket, but seems like this makes
sense as const.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.cc:1048: // Not implemented. Consider implementing
using
// TODO(bmcquade): Consider...

[Bryan McQuade, 2015-07-08 00:35:38]

Thanks for reviewing! RE: logging frequency, I agree that once at TCP connection
close probably isn't the right frequency to be representative of user requests.
I may plumb the RTT further up the stack at some point in the future to log once
per request, but I think this is a good starting point.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:183: void TCPClientSocket::Disconnect() {
On 2015/07/07 21:27:29, mmenke wrote:
> On 2015/07/07 21:03:32, Bryan McQuade wrote:
> > On 2015/07/07 19:53:49, mmenke wrote:
> > > Suggest a couple things:
> > > 
> > > 1) Rename this to EmitTCPMetricsHistograms (The "socket" is implied, and
> it's
> > > not clear what "close" means).
> > 
> > I changed to EmitTCPMetricsHistogramsOnDisconnect. I think it's important to
> > capture in the signature that the metric being recorded in the current
> > implementation only want to be recorded at disconnect time (the metric is
> named
> > Net.TcpRtt.AtDisconnect). Happy to remove the 'OnDisconnect' part of the
> method
> > name but I worry that it makes it easier for someone modifying this code in
> the
> > future to start calling the method at other locations, and inadvertently
> logging
> > Net.TcpRtt.AtDisconnect at places other than disconnection.
> > 
> > > 2) Move this to DoDisconnect.  We probably won't get anything in the other
> > 
> > > disconnect case, but the Disconnect / DoDisconnect split makes me very
> nervous
> > > about regressions.
> > 
> > Done. I noticed that DoDisconnect also gets called from
> > TCPClientSocket::DoConnectComplete with the comment " Close whatever
partially
> > connected socket we currently have.". I wasn't sure if it was desirable to
try
> > to emit histograms for such a socket. What do you think?
> 
> This is a valid concern.  While I suspect we'll see an estimated RTT of 0 in
> that case, I'm not confident of it.  One option would be to pass a bool into
> DoDisconnect indicating if we're calling it due to a connection failure or
not.

I'm ok leaving this as-is for the time being. To really improve things, I'll
likely need to plumb the RTT up to a higher layer in the stack to log at some
more sane interval like once per HTTP request.

> 
> > 
> > > 3) Call Disconnection on destruction.  We'll be doing a little extra work,
> but
> > > it looks pretty negligible to me.
> > 
> > Done. Note that Disconnect is virtual, so if TCPClientSocket is subclassed
and
> > Disconnect overridden, the overridden implementation won't get called when
the
> > derived class destructs (unless the derived class also calls Disconnect in
its
> > destructor...). Not sure if this is a concern or not. I could also call the
> > non-virtual DoDisconnect in the destructor. Let me know which you prefer.
> 
> The class really isn't meant to be subclassed - it's only done for tests. 
Some
> sticklers would doubtless disagree disagree with me on this, but I think it's
> fine as-is.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.cc:762: // When the kernel doesn't have an RTT
estimate, tcpi_rtt is zero.
On 2015/07/07 21:27:29, mmenke wrote:
> May be worth mentioning that it may be 0 in other cases, but most of those are
> probably localhost.

Done.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:67: // Gets the estimated RTT. Returns false if
the RTT is unavailable.
On 2015/07/07 21:27:29, mmenke wrote:
> Maybe add something like "May also return false when estimated RTT is 0."

Done.

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:68: bool
GetEstimatedRoundTripTime(base::TimeDelta* out_rtt);
On 2015/07/07 21:27:29, mmenke wrote:
> Can this be const?  (And the method that calls it).  Admittedly, calling
methods
> on socket descriptors can be const from a C++ perspective, but not logically
> const in terms of mutating the state of a socket, but seems like this makes
> sense as const.

Done.

[Bryan McQuade, 2015-07-08 12:17:42]

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/1215543003/diff/120001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.cc:1048: // Not implemented. Consider implementing
using
On 2015/07/07 21:27:29, mmenke wrote:
> // TODO(bmcquade): Consider...

Done.

[Jana, 2015-07-08 12:33:29]

Just one suggestion inline.

https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:69: bool
GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) const;
Since 0 is a special case that indicates failure, why not simply return 0 on
failure and non-zero values for all legit out values?

[Bryan McQuade, 2015-07-08 13:26:07]

https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_libevent.h:69: bool
GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) const;
On 2015/07/08 12:33:29, Jana wrote:
> Since 0 is a special case that indicates failure, why not simply return 0 on
> failure and non-zero values for all legit out values?

Thanks Jana! I'll defer to Matt on the decision here, but my thought is that
with a signature like 'RetVal Fn();' it's easy for callers to miss that zero
indicates failure, which means they end up incorrectly using the zero value as
the estimated RTT. This is really undesirable and could lead to bugs.

We can document this behavior in a comment in the header file, but devs don't
always consult headers - many times they see a method used elsewhere in the code
and start using it without consulting the header, so I think it's important for
the method's signature alone to clearly explain method behavior as much as
possible.

The 'bool Fn(Param* out)' signature is a little more cumbersome, but I claim
it's much harder for callers to mistakenly miss the failure case with a
signature like this.

I'm actually inclined to go a step further and add WARN_UNUSED_RESULT to the
signature, to make sure callers always observe the return value. I see this
pattern used elsewhere in net:
https://code.google.com/p/chromium/codesearch#search/&q=bool%5C%20.*WARN_UNUS...

What do you think?

[Bryan McQuade, 2015-07-08 13:37:02]

On 2015/07/08 13:26:07, Bryan McQuade wrote:
>
https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
> File net/socket/tcp_socket_libevent.h (right):
> 
>
https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
> net/socket/tcp_socket_libevent.h:69: bool
> GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) const;
> On 2015/07/08 12:33:29, Jana wrote:
> > Since 0 is a special case that indicates failure, why not simply return 0 on
> > failure and non-zero values for all legit out values?
> 
> Thanks Jana! I'll defer to Matt on the decision here, but my thought is that
> with a signature like 'RetVal Fn();' it's easy for callers to miss that zero
> indicates failure, which means they end up incorrectly using the zero value as
> the estimated RTT. This is really undesirable and could lead to bugs.
> 
> We can document this behavior in a comment in the header file, but devs don't
> always consult headers - many times they see a method used elsewhere in the
code
> and start using it without consulting the header, so I think it's important
for
> the method's signature alone to clearly explain method behavior as much as
> possible.
> 
> The 'bool Fn(Param* out)' signature is a little more cumbersome, but I claim
> it's much harder for callers to mistakenly miss the failure case with a
> signature like this.
> 
> I'm actually inclined to go a step further and add WARN_UNUSED_RESULT to the
> signature, to make sure callers always observe the return value. I see this
> pattern used elsewhere in net:
>
https://code.google.com/p/chromium/codesearch#search/&q=bool%5C%20.*WARN_UNUS...
> 
> What do you think?

Just updated the patch to include WARN_UNUSED_RESULT. PTAL.

[mmenke, 2015-07-08 15:16:25]

On 2015/07/08 13:26:07, Bryan McQuade wrote:
>
https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
> File net/socket/tcp_socket_libevent.h (right):
> 
>
https://codereview.chromium.org/1215543003/diff/140001/net/socket/tcp_socket_...
> net/socket/tcp_socket_libevent.h:69: bool
> GetEstimatedRoundTripTime(base::TimeDelta* out_rtt) const;
> On 2015/07/08 12:33:29, Jana wrote:
> > Since 0 is a special case that indicates failure, why not simply return 0 on
> > failure and non-zero values for all legit out values?
> 
> Thanks Jana! I'll defer to Matt on the decision here, but my thought is that
> with a signature like 'RetVal Fn();' it's easy for callers to miss that zero
> indicates failure, which means they end up incorrectly using the zero value as
> the estimated RTT. This is really undesirable and could lead to bugs.
> 
> We can document this behavior in a comment in the header file, but devs don't
> always consult headers - many times they see a method used elsewhere in the
code
> and start using it without consulting the header, so I think it's important
for
> the method's signature alone to clearly explain method behavior as much as
> possible.
> 
> The 'bool Fn(Param* out)' signature is a little more cumbersome, but I claim
> it's much harder for callers to mistakenly miss the failure case with a
> signature like this.
> 
> I'm actually inclined to go a step further and add WARN_UNUSED_RESULT to the
> signature, to make sure callers always observe the return value. I see this
> pattern used elsewhere in net:
>
https://code.google.com/p/chromium/codesearch#search/&q=bool%5C%20.*WARN_UNUS...
> 
> What do you think?

I agree with Bryan here (As we discussed offline yesterday).  Think returning a
bool makes misuse much less likely, and also allows us to change behavior to
return 0 in the future, without updating all callsites (I don't see us doing so,
but I prefer more flexible APIs).

LGTM

[Bryan McQuade, 2015-07-08 15:24:20]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2015-07-08 15:24:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1215543003/#ps180001
(title: "Add WARN_UNUSED_RESULT.")

[commit-bot: I haz the power, 2015-07-08 15:25:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215543003/180001

[commit-bot: I haz the power, 2015-07-08 16:04:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-08 16:04:56]

Patchset 10 (id:??) landed as
https://crrev.com/8b62f477c62822a4419298f690ae06907544f516
Cr-Commit-Position: refs/heads/master@{#337827}