Add P2PDatagramSocket and P2PStreamSocket interfaces.

Add P2PDatagramSocket and P2PStreamSocket interfaces.

Previously remoting code was using net::Socket and net::StreamSocket
for datagram and stream socket. Problem is that net::StreamSocket
interface contains a lot of methods that are not relevant for
peer-to-peer connections in remoting. Added P2PDatagramSocket and
P2PStreamSocket interfaces independent of net::Socket. This allowed to
remove a lot of the redundant code needed for net::StreamSocket
implementations. There are two new adapters required in
SslHmacChannelAuthenticator for the SSL layer, but these won't be
necessary after we migrate to QUIC.

Committed: https://crrev.com/aa22c0858c81fe75f72c1706cf052af85a82b364
Cr-Commit-Position: refs/heads/master@{#339489}

[Sergey Ulanov, 2015-06-19 21:07:30]

sergeyu@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2015-06-20 00:14:23]

Is there any value in having them inherit from net::Socket?  Fundamentally
datagram and stream sockets have different read/write semantics, so calling
code always has to know which it's dealing with anyway, surely?

On 19 June 2015 at 14:07, <sergeyu@chromium.org> wrote:

> Reviewers: Wez,
>
> Description:
> Add P2PDatagramSocket and P2PStreamSocket interfaces.
>
> Previously remoting code was using net::Socket and net::StreamSocket
> for datagram and stream socket. Problem is that net::StreamSocket
> interface contains a lot of methods that are not relevant for
> peer-to-peer connections in remoting. Added P2PDatagramSocket and
> P2PStreamSocket interfaces. Both interfaces inherit from net::Socket
> without adding any new methods. This allowed to remove a lot of the
> redundant code needed for net::StreamSocket implementations. There are
> two new adapters required in SslHmacChannelAuthenticator for the SSL
> layer, but these won't be necessary after we migrate to QUIC.
>
> Please review this at https://codereview.chromium.org/1197853003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+304, -461 lines):
>   M remoting/protocol/authenticator_test_base.h
>   M remoting/protocol/authenticator_test_base.cc
>   M remoting/protocol/channel_authenticator.h
>   M remoting/protocol/channel_dispatcher_base.h
>   M remoting/protocol/channel_dispatcher_base.cc
>   M remoting/protocol/channel_multiplexer.h
>   M remoting/protocol/channel_multiplexer.cc
>   M remoting/protocol/channel_multiplexer_unittest.cc
>   M remoting/protocol/channel_socket_adapter.h
>   M remoting/protocol/connection_tester.h
>   M remoting/protocol/connection_tester.cc
>   M remoting/protocol/datagram_channel_factory.h
>   M remoting/protocol/fake_authenticator.h
>   M remoting/protocol/fake_authenticator.cc
>   M remoting/protocol/fake_datagram_socket.h
>   M remoting/protocol/fake_stream_socket.h
>   M remoting/protocol/fake_stream_socket.cc
>   M remoting/protocol/jingle_session.h
>   M remoting/protocol/jingle_session_unittest.cc
>   M remoting/protocol/libjingle_transport_factory.cc
>   M remoting/protocol/message_reader.h
>   M remoting/protocol/message_reader.cc
>   A remoting/protocol/p2p_socket.h
>   M remoting/protocol/pseudotcp_adapter.h
>   M remoting/protocol/pseudotcp_adapter.cc
>   M remoting/protocol/pseudotcp_adapter_unittest.cc
>   M remoting/protocol/pseudotcp_channel_factory.h
>   M remoting/protocol/pseudotcp_channel_factory.cc
>   M remoting/protocol/secure_channel_factory.h
>   M remoting/protocol/secure_channel_factory.cc
>   M remoting/protocol/ssl_hmac_channel_authenticator.h
>   M remoting/protocol/ssl_hmac_channel_authenticator.cc
>   M remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc
>   M remoting/protocol/stream_channel_factory.h
>   M remoting/protocol/transport.h
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2015-06-22 15:49:07]

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/authentic...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/authentic...
remoting/protocol/authenticator_test_base.cc:17: #include
"remoting/protocol/fake_stream_socket.h"
nit: include p2p_socket.h header, since I think you need the concrete definition
for the scoped_ptr<P2PSocket> code below to compile.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:122: int SetReceiveBufferSize(int32
size) override {
We wouldn't need these if we didn't derive from net::Socket at all!

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/fake_stre...
File remoting/protocol/fake_stream_socket.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/fake_stre...
remoting/protocol/fake_stream_socket.cc:34: if (peer_socket_.get()) {
nit: Remove .get()

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:18: #include
"remoting/protocol/p2p_socket.h"
Doesn't look like you need this include here?

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/p2p_socket.h
File remoting/protocol/p2p_socket.h (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/p2p_socke...
remoting/protocol/p2p_socket.h:14: class P2PStreamSocket : public net::Socket
{};
As noted before, is there any value in deriving from net::Socket?

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
File remoting/protocol/pseudotcp_adapter.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
remoting/protocol/pseudotcp_adapter.cc:15: #include "net/base/net_util.h"
You need the p2p_socket.h include here, I think.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
File remoting/protocol/pseudotcp_adapter.h (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
remoting/protocol/pseudotcp_adapter.h:21: // PseudoTcp. Because
net::StreamSockets can be deleted during callbacks,
nit: Because P2PStreamSockets...

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/ssl_hmac_...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/ssl_hmac_...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:20: #include
"remoting/protocol/fake_session.h"
Need the p2p_socket.h include here?

[Sergey Ulanov, 2015-07-10 00:49:55]

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/authentic...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/authentic...
remoting/protocol/authenticator_test_base.cc:17: #include
"remoting/protocol/fake_stream_socket.h"
On 2015/06/22 15:49:06, Wez wrote:
> nit: include p2p_socket.h header, since I think you need the concrete
definition
> for the scoped_ptr<P2PSocket> code below to compile.

Done.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:122: int SetReceiveBufferSize(int32
size) override {
On 2015/06/22 15:49:07, Wez wrote:
> We wouldn't need these if we didn't derive from net::Socket at all!

Done.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/fake_stre...
File remoting/protocol/fake_stream_socket.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/fake_stre...
remoting/protocol/fake_stream_socket.cc:34: if (peer_socket_.get()) {
On 2015/06/22 15:49:07, Wez wrote:
> nit: Remove .get()

Done.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:18: #include
"remoting/protocol/p2p_socket.h"
On 2015/06/22 15:49:07, Wez wrote:
> Doesn't look like you need this include here?

Done.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/p2p_socket.h
File remoting/protocol/p2p_socket.h (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/p2p_socke...
remoting/protocol/p2p_socket.h:14: class P2PStreamSocket : public net::Socket
{};
On 2015/06/22 15:49:07, Wez wrote:
> As noted before, is there any value in deriving from net::Socket?

No. Rewrote these so they don't depend on net and now P2PStreamSocket and 
P2PDatagramSocket  are not related to each other. The only problem was that they
were used with BufferedSocketWriter which we still want to use with regular TCP
socket - solved that by passing Write() callback to BufferedSocketWriter instead
of net::Socket*.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
File remoting/protocol/pseudotcp_adapter.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
remoting/protocol/pseudotcp_adapter.cc:15: #include "net/base/net_util.h"
On 2015/06/22 15:49:07, Wez wrote:
> You need the p2p_socket.h include here, I think.

it's included in pseudotcp_adapter.h

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
File remoting/protocol/pseudotcp_adapter.h (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/pseudotcp...
remoting/protocol/pseudotcp_adapter.h:21: // PseudoTcp. Because
net::StreamSockets can be deleted during callbacks,
On 2015/06/22 15:49:07, Wez wrote:
> nit: Because P2PStreamSockets...

Done.

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/ssl_hmac_...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/1197853003/diff/1/remoting/protocol/ssl_hmac_...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:20: #include
"remoting/protocol/fake_session.h"
On 2015/06/22 15:49:07, Wez wrote:
> Need the p2p_socket.h include here?

Done.

[Wez, 2015-07-10 01:18:24]

https://codereview.chromium.org/1197853003/diff/40001/remoting/base/buffered_...
File remoting/base/buffered_socket_writer.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.h:45: const base::Closure& done_task);
Not necessarily for this CL, but this class seems overly complex; we could
replace it with separate Stream and Datagram implementations (the Datagram one
just queues the buffers and sends them; no need to track positions within
buffers) and have those implement P2PStreamSocket/DatagramSocket respectively -
WDYT?

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:29: // completes, the callback will not
be invoked.
What happens if a packet larger than |buf_len| is received? Is that an error? Or
will it be truncated? Or not returned at all?

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:30: virtual int Recv(net::IOBuffer* buf,
int buf_len,
Should these be scoped_refptr<>&, here and in the streamsocket interface?

[Sergey Ulanov, 2015-07-10 20:48:57]

https://codereview.chromium.org/1197853003/diff/40001/remoting/base/buffered_...
File remoting/base/buffered_socket_writer.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.h:45: const base::Closure& done_task);
On 2015/07/10 01:18:24, Wez wrote:
> Not necessarily for this CL, but this class seems overly complex; we could
> replace it with separate Stream and Datagram implementations (the Datagram one
> just queues the buffers and sends them; no need to track positions within
> buffers) and have those implement P2PStreamSocket/DatagramSocket respectively
-
> WDYT?

Simplified this code now. BufferedDatagramWriter was never used (and there were
no tests). I don't think we want to implement P2PStreamSocket here because this
class deals only with writing and Write() semantics is different.

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:29: // completes, the callback will not
be invoked.
On 2015/07/10 01:18:24, Wez wrote:
> What happens if a packet larger than |buf_len| is received? Is that an error?
Or
> will it be truncated? Or not returned at all?

Added a separate comment for Send().

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:30: virtual int Recv(net::IOBuffer* buf,
int buf_len,
On 2015/07/10 01:18:24, Wez wrote:
> Should these be scoped_refptr<>&, here and in the streamsocket interface?

Done.

[Sergey Ulanov, 2015-07-14 16:13:55]

ping

[Wez, 2015-07-14 17:12:53]

nit: Can you reformat the CL description to more readable - the big chunk of
text makes it hard to skim the list of changes in the CL!

[Wez, 2015-07-14 18:28:09]

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:29: // completes, the callback will not
be invoked.
On 2015/07/10 20:48:57, Sergey Ulanov wrote:
> On 2015/07/10 01:18:24, Wez wrote:
> > What happens if a packet larger than |buf_len| is received? Is that an
error?
> Or
> > will it be truncated? Or not returned at all?
> 
> Added a separate comment for Send().

That comment doesn't clarify the Recv() behaviour if an incoming packet exceeds
|buf_len| in size, though?

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:100: base::Unretained(this)));
You're using base::Unretained() here, but this class doesn't own the supplied
|socket|, so there's no explicit guarantee that a queued Write() will be dropped
when necessary.

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:114: closed_ = true;
nit: Could you instead implement a private getter is_closed() that checks
|write_callback_|, to ensure consistent internal state?

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:115: STLDeleteElements(&queue_);
Why do we need to delete the elements here? Surely we're not going to write any
more anyway?

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:131: destroyed_flag_ = &destroyed;
I realise that this was in the original implementation, but it seems super ugly
- do we really need the caller to be able to delete the buffered writer in
response to the write-completion callback? Do we really need a write-completion
callback per-buffer, even, or could we batch it?

If so, given that write completion is already asynchronous, could we defer the
callback, or the continuation of writes, via PostTask?

If that sounds evil, then consider turning DoWrite/HandleWriteResult into a
tail-recursive cycle, and using a WeakPtr to do the HandleWriteResult->DoWrite
call; that is equivalent to the pointer-to-bool but more familiar to folks
reading this code.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:33: // Sends a packet. Size of the
incoming packet is returned in case of success.
nit: This is send; the packet is outgoing, not incoming.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:34: // If the packet is larger than
|buf_len| then it's truncated, i.e. only the
nit: it's -> it is

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:35: // first |buf_len| bytes are stored
in the buffer. In case of a failure an
You mean that only |buf_len| bytes are written?

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:35: // first |buf_len| bytes are stored
in the buffer. In case of a failure an
nit: "In case of failure a net error code is returned..."

[Sergey Ulanov, 2015-07-14 21:07:54]

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/40001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:29: // completes, the callback will not
be invoked.
On 2015/07/14 18:28:09, Wez wrote:
> On 2015/07/10 20:48:57, Sergey Ulanov wrote:
> > On 2015/07/10 01:18:24, Wez wrote:
> > > What happens if a packet larger than |buf_len| is received? Is that an
> error?
> > Or
> > > will it be truncated? Or not returned at all?
> > 
> > Added a separate comment for Send().
> 
> That comment doesn't clarify the Recv() behaviour if an incoming packet
exceeds
> |buf_len| in size, though?

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:100: base::Unretained(this)));
On 2015/07/14 18:28:09, Wez wrote:
> You're using base::Unretained() here, but this class doesn't own the supplied
> |socket|, so there's no explicit guarantee that a queued Write() will be
dropped
> when necessary.

Good point. Fixed by using weak pointer here.

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:114: closed_ = true;
On 2015/07/14 18:28:09, Wez wrote:
> nit: Could you instead implement a private getter is_closed() that checks
> |write_callback_|, to ensure consistent internal state?

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:115: STLDeleteElements(&queue_);
On 2015/07/14 18:28:09, Wez wrote:
> Why do we need to delete the elements here? Surely we're not going to write
any
> more anyway?

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/base/buffered_...
remoting/base/buffered_socket_writer.cc:131: destroyed_flag_ = &destroyed;
On 2015/07/14 18:28:09, Wez wrote:
> I realise that this was in the original implementation, but it seems super
ugly
> - do we really need the caller to be able to delete the buffered writer in
> response to the write-completion callback?

I'm not sure we need it right now, but I think we needed it in the past and in
general I prefer when callback allow to delete the calling object. E.g. it
allows to create a writer to write a single message and delete it right after
that.

> Do we really need a write-completion
> callback per-buffer, even, or could we batch it?

Not sure what you mean by batching. 

> 
> If so, given that write completion is already asynchronous, could we defer the
> callback, or the continuation of writes, via PostTask?

Yes. Changed this code to call done_task and post async task for DoWrite().

> 
> If that sounds evil, then consider turning DoWrite/HandleWriteResult into a
> tail-recursive cycle, and using a WeakPtr to do the HandleWriteResult->DoWrite
> call; that is equivalent to the pointer-to-bool but more familiar to folks
> reading this code.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
File remoting/protocol/p2p_datagram_socket.h (right):

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:33: // Sends a packet. Size of the
incoming packet is returned in case of success.
On 2015/07/14 18:28:09, Wez wrote:
> nit: This is send; the packet is outgoing, not incoming.

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:34: // If the packet is larger than
|buf_len| then it's truncated, i.e. only the
On 2015/07/14 18:28:09, Wez wrote:
> nit: it's -> it is

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:35: // first |buf_len| bytes are stored
in the buffer. In case of a failure an
On 2015/07/14 18:28:09, Wez wrote:
> nit: "In case of failure a net error code is returned..."

Done.

https://codereview.chromium.org/1197853003/diff/80001/remoting/protocol/p2p_d...
remoting/protocol/p2p_datagram_socket.h:35: // first |buf_len| bytes are stored
in the buffer. In case of a failure an
On 2015/07/14 18:28:09, Wez wrote:
> You mean that only |buf_len| bytes are written?

Done.

[Wez, 2015-07-14 21:14:03]

Did you mean to upload a new patch-set?

[Sergey Ulanov, 2015-07-14 21:16:04]

I did. A minute later after posting the reply. Sorry.

On Tue, Jul 14, 2015 at 2:14 PM, <wez@chromium.org> wrote:

> Did you mean to upload a new patch-set?
>
> https://codereview.chromium.org/1197853003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2015-07-14 21:49:20]

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
Now that you're using WeakPtr you don't even need to PostTask - you can just
take a copy of the WeakPtr and test it immediately after running the
|done_task|, to decide whether to call DoWrite() before you return?

If you're going to call DoWrite() here as I suggested, then you don't want a
write loop in DoWrite() itself - just let it call HandleWriteResult and let that
call back round to DoWrite().

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:145: *write_again = true;
Why is |write_again| an out parameter rather than a return value? Or, actually
with DoWrite() called in HandleWriteResult you can get ridof this entirely, I
think?

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:156: DoWrite();
As noted above, if you have HandleWriteResult() call DoWrite via WeakPtr then
you don't need this.

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.h (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.h:73:
base::WeakPtrFactory<BufferedSocketWriter> weak_factory_;
Looks like this class is missing DISALLOW_COPY_AND_ASSIGN?

[Sergey Ulanov, 2015-07-14 22:53:21]

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
On 2015/07/14 21:49:20, Wez wrote:
> Now that you're using WeakPtr you don't even need to PostTask - you can just
> take a copy of the WeakPtr and test it immediately after running the
> |done_task|, to decide whether to call DoWrite() before you return?
> 
> If you're going to call DoWrite() here as I suggested, then you don't want a
> write loop in DoWrite() itself - just let it call HandleWriteResult and let
that
> call back round to DoWrite().

We don't want to call DoWrite() directly here because it can potentially
overflow the stack. I.e. DoWrite() calls HandleWriteResult() and
HandleWriteResult() will be calling DoWrite()

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:145: *write_again = true;
On 2015/07/14 21:49:20, Wez wrote:
> Why is |write_again| an out parameter rather than a return value? 
Just for readability, to make the purpose of the result clear.

> Or, actually
> with DoWrite() called in HandleWriteResult you can get rid of this entirely, I
> think?

Done, but this comes at cost of posting a task every time in
HandleWriteResult(), even when it can be avoided.

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:156: DoWrite();
On 2015/07/14 21:49:20, Wez wrote:
> As noted above, if you have HandleWriteResult() call DoWrite via WeakPtr then
> you don't need this.

Done.

[Sergey Ulanov, 2015-07-16 16:40:05]

ping

[Wez, 2015-07-16 21:55:36]

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> On 2015/07/14 21:49:20, Wez wrote:
> > Now that you're using WeakPtr you don't even need to PostTask - you can just
> > take a copy of the WeakPtr and test it immediately after running the
> > |done_task|, to decide whether to call DoWrite() before you return?
> > 
> > If you're going to call DoWrite() here as I suggested, then you don't want a
> > write loop in DoWrite() itself - just let it call HandleWriteResult and let
> that
> > call back round to DoWrite().
> 
> We don't want to call DoWrite() directly here because it can potentially
> overflow the stack. I.e. DoWrite() calls HandleWriteResult() and
> HandleWriteResult() will be calling DoWrite()

If it's the last thing that HandleWriteResult() calls then won't the compiler
optimize that to use tail recursion, though?

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:145: *write_again = true;
On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> On 2015/07/14 21:49:20, Wez wrote:
> > Why is |write_again| an out parameter rather than a return value? 
> Just for readability, to make the purpose of the result clear.
> 
> > Or, actually
> > with DoWrite() called in HandleWriteResult you can get rid of this entirely,
I
> > think?
> 
> Done, but this comes at cost of posting a task every time in
> HandleWriteResult(), even when it can be avoided.

You don't need the PostTask, though - you can just take a WeakPtr before you
call |done_task| and then check it afterward, and if it's still valid, then call
on to DoWrite() directly, as the last thing you do, so that the compiler can
optimize it to a tail recursion?

The alternative would be to have HandleWriteResult() return a bool that is false
if the caller should immediately pop its stack - much like the previous
|write_again|, except that you'd have HandleWriteResult return |weak_ptr_|
coerced to bool in the case where there |might| be something more to write. So
basically like the old code but without the strange out-value and the
pointer-to-bool hack.

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.h (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.h:73:
base::WeakPtrFactory<BufferedSocketWriter> weak_factory_;
On 2015/07/14 21:49:20, Wez wrote:
> Looks like this class is missing DISALLOW_COPY_AND_ASSIGN?

Ping.

[Sergey Ulanov, 2015-07-16 22:11:03]

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
On 2015/07/16 21:55:36, Wez wrote:
> On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> > On 2015/07/14 21:49:20, Wez wrote:
> > > Now that you're using WeakPtr you don't even need to PostTask - you can
just
> > > take a copy of the WeakPtr and test it immediately after running the
> > > |done_task|, to decide whether to call DoWrite() before you return?
> > > 
> > > If you're going to call DoWrite() here as I suggested, then you don't want
a
> > > write loop in DoWrite() itself - just let it call HandleWriteResult and
let
> > that
> > > call back round to DoWrite().
> > 
> > We don't want to call DoWrite() directly here because it can potentially
> > overflow the stack. I.e. DoWrite() calls HandleWriteResult() and
> > HandleWriteResult() will be calling DoWrite()
> 
> If it's the last thing that HandleWriteResult() calls then won't the compiler
> optimize that to use tail recursion, though?

AFAIK some C++ compilers implement tail call optimization, but it's not required
by the standard so we cannot rely on it here. E.g. it's not enabled in debug
builds. It also may be broken by other changes, e.g. making this method virtual.

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:145: *write_again = true;
On 2015/07/16 21:55:36, Wez wrote:
> On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> > On 2015/07/14 21:49:20, Wez wrote:
> > > Why is |write_again| an out parameter rather than a return value? 
> > Just for readability, to make the purpose of the result clear.
> > 
> > > Or, actually
> > > with DoWrite() called in HandleWriteResult you can get rid of this
entirely,
> I
> > > think?
> > 
> > Done, but this comes at cost of posting a task every time in
> > HandleWriteResult(), even when it can be avoided.
> 
> You don't need the PostTask, though - you can just take a WeakPtr before you
> call |done_task| and then check it afterward, and if it's still valid, then
call
> on to DoWrite() directly, as the last thing you do, so that the compiler can
> optimize it to a tail recursion?
> 
> The alternative would be to have HandleWriteResult() return a bool that is
false
> if the caller should immediately pop its stack - much like the previous
> |write_again|, except that you'd have HandleWriteResult return |weak_ptr_|
> coerced to bool in the case where there |might| be something more to write. So
> basically like the old code but without the strange out-value and the
> pointer-to-bool hack.

So if I understand correctly you are suggesting that HandleWriteResult() should
return a bool and a weak_ptr. Is that right interpretation?

[Sergey Ulanov, 2015-07-16 22:25:57]

I've reverted BufferedSocketWriter to what it was in patchset 6. Cleaning up
BufferedSocketWriter isn't a goal of this CL, so I'd prefer leaving all further
cleanups to a separate CL.

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
File remoting/base/buffered_socket_writer.cc (right):

https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
On 2015/07/16 22:11:03, Sergey Ulanov wrote:
> On 2015/07/16 21:55:36, Wez wrote:
> > On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> > > On 2015/07/14 21:49:20, Wez wrote:
> > > > Now that you're using WeakPtr you don't even need to PostTask - you can
> just
> > > > take a copy of the WeakPtr and test it immediately after running the
> > > > |done_task|, to decide whether to call DoWrite() before you return?
> > > > 
> > > > If you're going to call DoWrite() here as I suggested, then you don't
want
> a
> > > > write loop in DoWrite() itself - just let it call HandleWriteResult and
> let
> > > that
> > > > call back round to DoWrite().
> > > 
> > > We don't want to call DoWrite() directly here because it can potentially
> > > overflow the stack. I.e. DoWrite() calls HandleWriteResult() and
> > > HandleWriteResult() will be calling DoWrite()
> > 
> > If it's the last thing that HandleWriteResult() calls then won't the
compiler
> > optimize that to use tail recursion, though?
> 
> AFAIK some C++ compilers implement tail call optimization, but it's not
required
> by the standard so we cannot rely on it here. E.g. it's not enabled in debug
> builds. It also may be broken by other changes, e.g. making this method
virtual.

Also there are cases when tail call optimization is just not possible in C++,
e.g.:

int foo() {
  A a;
  return bar();
}

in this case ~A() must be called after bar().

[Wez, 2015-07-16 22:28:35]

On 2015/07/16 22:11:03, Sergey Ulanov wrote:
>
https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
> File remoting/base/buffered_socket_writer.cc (right):
> 
>
https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
> remoting/base/buffered_socket_writer.cc:139: weak_factory_.GetWeakPtr()));
> On 2015/07/16 21:55:36, Wez wrote:
> > On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> > > On 2015/07/14 21:49:20, Wez wrote:
> > > > Now that you're using WeakPtr you don't even need to PostTask - you can
> just
> > > > take a copy of the WeakPtr and test it immediately after running the
> > > > |done_task|, to decide whether to call DoWrite() before you return?
> > > > 
> > > > If you're going to call DoWrite() here as I suggested, then you don't
want
> a
> > > > write loop in DoWrite() itself - just let it call HandleWriteResult and
> let
> > > that
> > > > call back round to DoWrite().
> > > 
> > > We don't want to call DoWrite() directly here because it can potentially
> > > overflow the stack. I.e. DoWrite() calls HandleWriteResult() and
> > > HandleWriteResult() will be calling DoWrite()
> > 
> > If it's the last thing that HandleWriteResult() calls then won't the
compiler
> > optimize that to use tail recursion, though?
> 
> AFAIK some C++ compilers implement tail call optimization, but it's not
required
> by the standard so we cannot rely on it here. E.g. it's not enabled in debug
> builds. It also may be broken by other changes, e.g. making this method
virtual.
> 
>
https://codereview.chromium.org/1197853003/diff/100001/remoting/base/buffered...
> remoting/base/buffered_socket_writer.cc:145: *write_again = true;
> On 2015/07/16 21:55:36, Wez wrote:
> > On 2015/07/14 22:53:20, Sergey Ulanov wrote:
> > > On 2015/07/14 21:49:20, Wez wrote:
> > > > Why is |write_again| an out parameter rather than a return value? 
> > > Just for readability, to make the purpose of the result clear.
> > > 
> > > > Or, actually
> > > > with DoWrite() called in HandleWriteResult you can get rid of this
> entirely,
> > I
> > > > think?
> > > 
> > > Done, but this comes at cost of posting a task every time in
> > > HandleWriteResult(), even when it can be avoided.
> > 
> > You don't need the PostTask, though - you can just take a WeakPtr before you
> > call |done_task| and then check it afterward, and if it's still valid, then
> call
> > on to DoWrite() directly, as the last thing you do, so that the compiler can
> > optimize it to a tail recursion?
> > 
> > The alternative would be to have HandleWriteResult() return a bool that is
> false
> > if the caller should immediately pop its stack - much like the previous
> > |write_again|, except that you'd have HandleWriteResult return |weak_ptr_|
> > coerced to bool in the case where there |might| be something more to write.
So
> > basically like the old code but without the strange out-value and the
> > pointer-to-bool hack.
> 
> So if I understand correctly you are suggesting that HandleWriteResult()
should
> return a bool and a weak_ptr. Is that right interpretation?

No, it can just return a bool and if that's false then the caller (OnWritten or
DoWrite) must immediately drop out to a clean stack when it returns.

Internally, HandleWriteResult() would look like:

bool HandleWriteResult(...) {
  .. proces the result ..
  WeakPtr weak_this = weak_ptr_;
  done_task.Run();
  return weak_this;
}

|weak_this| will be false, in the same way as the old bool-on-the-stack was,
thereby protecting against deletion within |done_task|.

If you want to be more explicit about it then you could have the callers
(OnWritten & DoWrite) take a copy of the WeakPtr to the stack and check it after
HandleWriteResult to determine whether to continue or return.

[Sergey Ulanov, 2015-07-16 22:52:32]

I've uploaded a new version that uses weak_ptr, but HandleWriteResult() doesn't
return anything. If you still disagree I'd prefer go back to the version I had
in patchset 6 and continue optimizing BufferedSocketWriter in a separate CL.
BufferedSocketWriter cleanup is not related to this CL.

[Wez, 2015-07-16 23:00:28]

Yes, you're right; apologise for ratholing this CL...

LGTM!

[Sergey Ulanov, 2015-07-16 23:01:44]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-07-16 23:04:14]

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

[commit-bot: I haz the power, 2015-07-17 00:04:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 00:04:14]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2015-07-17 00:08:18]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-07-17 00:09:57]

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

[commit-bot: I haz the power, 2015-07-17 01:07:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 01:07:11]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2015-07-20 17:45:20]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-07-20 17:45:20]

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

[commit-bot: I haz the power, 2015-07-20 17:46:02]

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

[commit-bot: I haz the power, 2015-07-20 19:41:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-20 19:42:43]

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