Add SocketWatcherFactory as a helper class to NQE

net/base/socket_performance_watcher.h

Index: net/base/socket_performance_watcher.h
diff --git a/net/base/socket_performance_watcher.h b/net/base/socket_performance_watcher.h
index e0e76327ce63541e8d0dc2ba23215d34f953df41..5b8566710321241a17ef2617ebb343014f556ecf 100644
--- a/net/base/socket_performance_watcher.h
+++ b/net/base/socket_performance_watcher.h
@@ -7,7 +7,6 @@
 
 #include "base/macros.h"
 #include "net/base/net_export.h"
-#include "net/base/socket_performance_watcher_factory.h"
 
 namespace base {
 class TimeDelta;
@@ -16,31 +15,32 @@ class TimeDelta;
 namespace net {
 
 // SocketPerformanceWatcher is the base class for recording and aggregating
-// socket statistics.
+// per-socket statistics. SocketPerformanceWatcher is not thread safe.

[Ryan Sleevi, 2016/04/02 00:48:20]

In a pure interface, you can't make this claim (that it's not thread safe)

Thread safety is either a detail of how it's used (e.g. it's called on multiple
threads) or how it's written (it's explicitly not safe to call on multiple
threads)

Because of this being in a standalone header, you shouldn't make any remarks
about how it is used OR implemented - those belong at either end of the
respective points (the socket consuming, or the implementation producing)


That said, during our meeting, we discussed making this an explicit (public)
member class of the Socket interfaces, since the behaviour is intrinsically
linked. I'm curious to understand the logic behind putting it in a separate
file.

The reason why I raise this is that there are pros and cons to this; pros is
that it facilitates better forward declaration, but the biggest con, especially
in the context of this series of reviews, is that it makes it very easy to
introduce subtle cyclic dependencies.

That said, this isn't a blanket "Go change this to that and I'm happy" - what
I'm trying to do is make sure I understand the design problem space, and am
mostly looking for a combination of sparking thought and encouraging dialog. For
example, in what contexts is RTT relevant?

Is it relevent to net/socket/socket? net/socket/stream_socket?
net/socket/tcp_socket? What about for QUIC - is this something that //net/quic
should depend on, or should //net/quic define its own RTT reporting mechanism?
QUIC already defines its own streaming interface 
(net/quic/reliable_quic_stream, the equivalent of stream_socket)

This is a big part of the open design question, and I'm not sure which answer is
right (although I have thoughts, naturally), but I want to make sure you're
thinking about these, and understand.

This is where the whiteboard may have helped - in most cases, many of the design
issues I've raised on this and the previous CL can be identified early on if you
draw out a dependency graph, where a given class (or directory) is a node, and
with dependencies as a directed edge between two nodes. The goal is to ensure
the graph always flows in a single direction, with no cycles, and is logically
coherent.

As such, it's unclear to me how this class fits in the Socket hierarchy, and as
we explore that, it will hopefully answer whether this should be something like
TcpStreamSocket::PerformanceDelegate or whether this does belong as a base class
(and the correct dependencies between //net/socket and //net/quic, which is a
separate conversation to have; ISTM that the answer is "//quic shouldn't depend
on //socket", but there a few outliers to that right now.

[tbansal1, 2016/04/04 16:55:44]

I thought it would be useful for the interface to make it clearer that the
implementation is not guaranteed to be thread safe so it's consumers
responsibility to call it in a safe manner. Should interfaces not export this
kind of information?
I think it is relevant to sockets that are capable of exposing performance
information. So, that would be socket and related classes (e.g., TCPSocketPosix,
TCPSocketWin, TCPServerSocket). It is also possible for UDP sockets to expose
performance information (via Datagram Congestion Control Protocol).
I think it should stay as a separate class (and not as something specific to
TcpStreamSocket) because performance information could be exposed by non-TCP
sockets too. I am torn between net/socket/ and net/base/. It seems that
net/socket is the right place for now, and eventually if the scope expands
beyond that (e.g., Websockets), then it should be move to net/base/.
I thought QUIC fundamentally depends on //net/socket because it depends on
DatagramClientSocket which implements Socket.

[Ryan Sleevi, 2016/04/04 17:30:09]

If you're expressing requirements, then express them as requirements, not
guarantees.

That is, "implementations must be thread-safe", rather than "this IS thread
safe"

However, when defining an interface, it's important to think of "Why must this
be thread-safe". Because you've chosen to decouple this from socket, the
reasoning for that pre-condition is less clear, and it can change (e.g. if we
change how EmbeddedTestServer works, there's no longer a need for this to be
thread-safe). That's always a challenge - trying to figure out "Why condition X"
and figuring out whether that should be expressed on the base class / interface
(All instances must X) or at the receiver (e.g. all instances you use with this
method must be X).
But would UDP sockets expose RTT information? Is *this* information and
interface correct for all sockets?

For example, a UDP socket may be receiving packets from multiple senders (e.g.
not bound to a strict 4-tuple). So I think a UDP performance interface MAY need
to look different, right?

Let's focus on the concrete here - I agree many sockets can provide "performance
information", the question is whether they provide the same set of information,
and with the same set of preconditions/post-conditions. That's what I was
unclear of.
You're right - I missed that (I had only looked at the //net/udp base classes).
Since //net/quic depends on //net/udp which depends on //net/socket, it should
be fine to make //net/quic depend on //net/socket directly, I totally agree.

[tbansal1, 2016/04/04 18:35:46]

Done. The requirement comes from SocketWatcher not thread safe. Currently, it is
unrelated to EmbeddedTestServer thread hopping.
Right, the current concrete case is TCPSocket. I still think SPW should go as a
separate file in net/socket/. It needs to be accessed by  TCPSocketPosix,
TCPClientSocket, and all classes above it (SocketPool classes). TCPSocketPosix
is at the bottom of the hierarchy, but is included only on POSIX platforms
(TCPSocketWin is included otherwise). So, I can't add it in the TCPSocketPosix
(because then it won't be included on non-POSIX platforms). I also can't include
it in higher -layer classes because then TCPSocketPosix would have a dependency
on higher layer classes.

[Ryan Sleevi, 2016/04/04 18:53:36]

I'm not sure I follow this logic. If this is the case, why isn't
TCPSocket::PerformanceDelegate?

That's more of the question at hand - whether it goes lower (which a separate
file encourages), or whether this interface is specific to TCP sockets, and a
different interface (DatagramSocket::PerformanceDelegate, etc) is appropriate
for the others?

[tbansal1, 2016/04/04 22:07:02]

TCPSocket is a reasonable place for now. I can move it in the next CL?

[Ryan Sleevi, 2016/04/04 22:34:04]

SG

 class NET_EXPORT_PRIVATE SocketPerformanceWatcher {
  public:
-  // |socket_performance_watcher_factory| is the factory that constructed
-  // |this| watcher.
-  SocketPerformanceWatcher(
-      const SocketPerformanceWatcherFactory::Protocol protocol,
-      SocketPerformanceWatcherFactory* socket_performance_watcher_factory);
+  ~SocketPerformanceWatcher() {}
 
-  virtual ~SocketPerformanceWatcher();
+  // Notifies |this| SocketPerformanceWatcher of updated transport layer RTT
+  // from this device to the remote transport layer endpoint. This method is
+  // called immediately after the observation is made, hence no timestamp.
+  // There is no guarantee that OnUpdatedRTTAvailable will be called every time
+  // an updated RTT is available as the socket may throttle
+  // OnUpdatedRTTAvailable call for various reasons (including performance).
+  virtual void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) = 0;
 
-  // Called when updated transport layer RTT information is available. This
-  // must be the transport layer RTT from this device to the remote transport
-  // layer endpoint. This method is called immediately after the observation is
-  // made, hence no timestamp.
-  void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) const;
+  // Returns true if |this| SocketPerformanceWatcher is interested in receiving
+  // an updated RTT estimate (via OnUpdatedRTTAvailable).
+  virtual bool ShouldNotifyUpdatedRTT() const = 0;
 
- private:
-  // Transport layer protocol used by the socket that |this| is watching.
-  const SocketPerformanceWatcherFactory::Protocol protocol_;
+  // Notifies that |this| watcher will be reused to watch a different socket.
+  // Note: The new socket shares the same protocol as the previously watched
+  // socket.
+  virtual void Reset() = 0;
 
-  // |socket_performance_watcher_factory_| is the factory that created
-  // |this| watcher.
-  SocketPerformanceWatcherFactory* socket_performance_watcher_factory_;
+ protected:
+  SocketPerformanceWatcher() {}

[Ryan Sleevi, 2016/04/02 00:48:20]

There's no need for this. It's a pure interface - you can't construct an
instance of this base type. If the virtual methods weren't pure virtual, then it
would argue it should be acceptable to create a (no-op) instance of this class.

[tbansal1, 2016/04/04 16:55:44]

Done.

 
+ private:
   DISALLOW_COPY_AND_ASSIGN(SocketPerformanceWatcher);
 };