[net/udp] Create UDPSocketWin::Core which persists until all network operations complete.

[net/udp] Create UDPSocketWin::Core which persists until all network operations complete.

The Core pattern is copied from TCPClientSocketWin.

BUG=121085
TEST=net_unittests --gtest_filter=UDP*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156328

[wtc, 2012-09-11 15:55:22]

rvargas: since you added TCPClientSocketWin::Core, could you
also review this CL?  Thanks.

[wtc, 2012-09-12 01:40:29]

Patch set 1 LGTM. Note that there are bugs and suggested
changes.

rvargas: I reviewed this CL carefully, so you don't need to
review it. But I have a question for you, and a comment on
WSABUF that may be of interest to you. Please search for
"rvargas".

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:56: WSABUF write_buffer_;

It turns out these two buffers don't need to be members of
Core. They can continue to be stack variables.

From the WSARecvFrom MSDN page:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741686%28v=vs.85%29...

  If this function is completed in an overlapped manner,
  it is the Winsock service provider's responsibility to
  capture the WSABUF structures before returning from this
  call. This enables applications to build stack-based
  WSABUF arrays pointed to by the lpBuffers parameter.

You don't need to make this change in this CL. You can
file a bug report and assign it to me or rvargas.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:97: // |read_watcher_| watches for events from
Connect() and Read().

Delete "Connect() and"?

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:195: if (core_.get())

Is this null pointer check necessary? It is not in
TCPClientSocketWin::DoDisconnect().

I think this check at the beginning of this function
  if (!is_connected())
    return;
should provide protection.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:479: IPEndPoint* address) {

I suggest copying
  DCHECK(!core_->read_iobuffer_);
from TCPClientSocketWin::Read().

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:485: core_ = new Core(this);

I think we can simply create Core in the UDPSocketWin constructor.

rvargas: do you remember why you didn't create Core in the
TCPClientSocketWin constructor?

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:486: core_->read_iobuffer_ = buf;

BUG: this can be a reference leak. You need to release this
reference if WSARecvFrom completes synchronously.

I suggest you copy TCPClientSocketWin::Read() and only do
  core_->read_iobuffer_ = buf;
before returning ERR_IO_PENDING.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:519: int UDPSocketWin::InternalSendTo(IOBuffer* buf,
int buf_len,

IMPORTANT: All of my comments for UDPSocketWin::InternalRecvFrom()
also apply to this method.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (left):

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.h#oldco...
net/udp/udp_socket_win.h:217: socklen_t recv_addr_len_;

BUG: recv_addr_storage_ and recv_addr_len_ need to be moved
to the Core class because they are passed to WSARecvFrom().

[szym, 2012-09-12 02:23:45]

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:195: if (core_.get())
On 2012/09/12 01:40:29, wtc wrote:
> 
> Is this null pointer check necessary? It is not in
> TCPClientSocketWin::DoDisconnect().
> 
> I think this check at the beginning of this function
>   if (!is_connected())
>     return;
> should provide protection.

It should, I added this in response to a failing test, but I didn't realize that
the socket is guaranteed to be either Connected or Bound, so the proper place to
create the Core is either in CreateSocket or in UDPSocketWin constructor.

I'll fix that.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:485: core_ = new Core(this);
On 2012/09/12 01:40:29, wtc wrote:
> 
> I think we can simply create Core in the UDPSocketWin constructor.
> 
> rvargas: do you remember why you didn't create Core in the
> TCPClientSocketWin constructor?

This will definitely be moved from here, either to the constructor or to
CreateSocket. Waiting for opinion from rvargas.

[wtc, 2012-09-12 04:00:43]

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:56: WSABUF write_buffer_;

On 2012/09/12 01:40:29, wtc wrote:
>
> You don't need to make this change in this CL. You can
> file a bug report and assign it to me or rvargas.

I have created a CL for TCPClientSocketWin:
http://codereview.chromium.org/10913212/

The WSASend and WSASendTo MSDN pages actually say something
contradictory, which I believe is wrong:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms742203%28v=vs.85%29...

lpBuffers [in]

    A pointer to an array of WSABUF structures. Each WSABUF structure contains a
pointer to a buffer and the length, in bytes, of the buffer. For a Winsock
application, once the WSASend function is called, the system owns these buffers
and the application may not access them. This array must remain valid for the
duration of the send operation.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:485: core_ = new Core(this);

On 2012/09/12 02:23:45, szym wrote:
>
> This will definitely be moved from here, either to the constructor or to
> CreateSocket. Waiting for opinion from rvargas.

I think TCPClientSocketWin creates Core lazily because
it needs to continue to use the object after Disconnect()
has been called. It is allowed to call Connect() again
after Disconnect().

If UDPSocketWin becomes "dead" after Close() is called,
then it is fine to create Core in the constructor or
CreateSocket(). Otherwise, it is best to create Core
lazily.

[szym, 2012-09-12 15:14:06]

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:56: WSABUF write_buffer_;
On 2012/09/12 04:00:43, wtc wrote:
> 
> On 2012/09/12 01:40:29, wtc wrote:
> >
> > You don't need to make this change in this CL. You can
> > file a bug report and assign it to me or rvargas.
> 
> I have created a CL for TCPClientSocketWin:
> http://codereview.chromium.org/10913212/
> 
> The WSASend and WSASendTo MSDN pages actually say something
> contradictory, which I believe is wrong:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms742203%2528v=vs.85%...
> 
> lpBuffers [in]
> 
>     A pointer to an array of WSABUF structures. Each WSABUF structure contains
a
> pointer to a buffer and the length, in bytes, of the buffer. For a Winsock
> application, once the WSASend function is called, the system owns these
buffers
> and the application may not access them. This array must remain valid for the
> duration of the send operation.
> 

That snippet from MSDN was the reason why I decided to put WSABUF in Core.
However, the relevant snippet about stack-based WSABUF arrays is present in both
WSARecvFrom and WSASendTo.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:97: // |read_watcher_| watches for events from
Connect() and Read().
On 2012/09/12 01:40:29, wtc wrote:
> 
> Delete "Connect() and"?

Done.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:479: IPEndPoint* address) {
On 2012/09/12 01:40:29, wtc wrote:
> 
> I suggest copying
>   DCHECK(!core_->read_iobuffer_);
> from TCPClientSocketWin::Read().

Done.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:485: core_ = new Core(this);
On 2012/09/12 04:00:43, wtc wrote:
> 
> On 2012/09/12 02:23:45, szym wrote:
> >
> > This will definitely be moved from here, either to the constructor or to
> > CreateSocket. Waiting for opinion from rvargas.
> 
> I think TCPClientSocketWin creates Core lazily because
> it needs to continue to use the object after Disconnect()
> has been called. It is allowed to call Connect() again
> after Disconnect().
> 
> If UDPSocketWin becomes "dead" after Close() is called,
> then it is fine to create Core in the constructor or
> CreateSocket(). Otherwise, it is best to create Core
> lazily.

It seems UDOSocketWin can be re-Connected or re-Bound after it's Closed. I moved
Core creation to CreateSocket.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:486: core_->read_iobuffer_ = buf;
On 2012/09/12 01:40:29, wtc wrote:
> 
> BUG: this can be a reference leak. You need to release this
> reference if WSARecvFrom completes synchronously.
> 
> I suggest you copy TCPClientSocketWin::Read() and only do
>   core_->read_iobuffer_ = buf;
> before returning ERR_IO_PENDING.

Done.

http://codereview.chromium.org/10918158/diff/1/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:519: int UDPSocketWin::InternalSendTo(IOBuffer* buf,
int buf_len,
On 2012/09/12 01:40:29, wtc wrote:
> 
> IMPORTANT: All of my comments for UDPSocketWin::InternalRecvFrom()
> also apply to this method.

Done.

[wtc, 2012-09-12 19:07:39]

Patch set 2 LGTM!

http://codereview.chromium.org/10918158/diff/4/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10918158/diff/4/net/udp/udp_socket_win.cc#newc...
net/udp/udp_socket_win.cc:482: SockaddrStorage& storage =
core_->recv_addr_storage_;

Nit: "recv_addr" is a better name than "storage" for this
variable. Also on line 607.

It is fine to not change this.