Keep listening if "accept" returns an invalid socket.

Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030
Committed: https://crrev.com/095ae15d6b9ff60357b44ed6f4997754079eff2e
Cr-Commit-Position: refs/heads/master@{#11080}

[joachim, 2015-11-17 23:20:52]

jbauch@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[joachim, 2015-11-17 23:20:53]

Ptal

[pthatcher, 2015-11-19 22:41:52]

On 2015/11/17 23:20:53, joachim wrote:
> Ptal

I don't quite understand the bug or the fix.  Perhaps a unit test would clarify
(and we should have one anyway).

Basically the bug and fix are saying "I want to call Accept() and have it listen
for incoming connections instead of calling Listen()".  Why not just call
Listen()?  It causes enabled_events_ to have DE_ACCEPT added to it.  So why not
use Listen() to listen instead of Accept()?

[joachim, 2015-11-19 22:51:27]

On 2015/11/19 22:41:52, pthatcher wrote:
> On 2015/11/17 23:20:53, joachim wrote:
> > Ptal
> 
> I don't quite understand the bug or the fix.  Perhaps a unit test would
clarify
> (and we should have one anyway).
> 
> Basically the bug and fix are saying "I want to call Accept() and have it
listen
> for incoming connections instead of calling Listen()".  Why not just call
> Listen()?  It causes enabled_events_ to have DE_ACCEPT added to it.  So why
not
> use Listen() to listen instead of Accept()?

From what I understand, the bug is as follows:
- Application registers "SignalReadEvent" on PhysicalSocket
- Application calls Listen() on PhysicalSocket
- Client connects
-> OnEvent will be triggered with DE_ACCEPT
-> Event DE_ACCEPT will be cleared from enabled_events_
-> SignalReadEvent is called
- Application calls "Accept()" to get socket for client connection
- "::accept" fails and returns an invalid socket
-> Accept() returns without re-setting DE_ACCEPT on enabled_events_
-> Additional client connections will not caus OnEvent to fire

The fix is to always set DE_ACCEPT to continue getting OnEvent for new incoming
connections, even if "::accept" failed.

I tried writing a test for that, but couldn't find a way to make "::accept"
return an error on purpose.

[pthatcher1, 2015-11-19 23:16:26]

OK, that makes sense now.  

Could you explain something along these lines with a comment in the code? 
Knowledge like that is important to make sure it doesn't get moved around again
in some future change and breaks it again.

Even something as simple as "Future incoming connection will be ignored if
DE_ACCEPT is not set in enabled_events_, so we need to put it back every time we
call Accept(), not just when Accept succeeds".  

Also, that's are argument for making it the first line in the method.  Can you
move it all the way to the top?


Finally, you can test this by moving the call to ::accept into a protected
method, then subclass PhysicalSocket in the unit test and override that method
to return null/false/error when you need it to.  It's work, but it will make
sure we don't accidentally re-break this sometime in the future.

[joachim, 2015-11-19 23:55:29]

On 2015/11/19 23:16:26, pthatcher1 wrote:
> OK, that makes sense now.  
> 
> Could you explain something along these lines with a comment in the code? 
> Knowledge like that is important to make sure it doesn't get moved around
again
> in some future change and breaks it again.
> 
> Even something as simple as "Future incoming connection will be ignored if
> DE_ACCEPT is not set in enabled_events_, so we need to put it back every time
we
> call Accept(), not just when Accept succeeds".  
> 
> Also, that's are argument for making it the first line in the method.  Can you
> move it all the way to the top?

Thanks for the feedback, I already changed that locally.

> Finally, you can test this by moving the call to ::accept into a protected
> method, then subclass PhysicalSocket in the unit test and override that method
> to return null/false/error when you need it to.  It's work, but it will make
> sure we don't accidentally re-break this sometime in the future.

To do that, I will have to move the class definition of PhysicalSocket into
physicalsocketserver.h as the class is currently only in physicalsocketserver.cc
and thus can't be subclassed from the tests. Is that ok to change?

[pthatcher1, 2015-11-20 00:05:36]

Hmmm... that sound painful.  Can you give it a try?  If it's too painful we can
backup and rethink.  If it's not, then it's fine to do so.

[joachim, 2015-11-20 00:23:47]

On 2015/11/20 00:05:36, pthatcher1 wrote:
> Hmmm... that sound painful.  Can you give it a try?  If it's too painful we
can
> backup and rethink.  If it's not, then it's fine to do so.

Depends on your definition of "painful" ;-) It's simple copy-paste grunt work,
I'll see when I get to do it. Maybe tomorrow or on the weekend.

[joachim, 2015-11-20 23:14:26]

As expected, the diff looks rather large, however I only changed a few things:
- Extracted classes "PhysicalSocket" and "SocketDispatcher" into header file, so
"DoAccept" can be overwritten from tests.
- Converted "NULL" to "nullptr" in code I touched.
- Added "GUARDED_BY" annotation to attribute in "PhysicalSocket".
- Added testcase to verify the socket keeps listening after "accept" returned an
error (based on "SocketTest::ConnectInternal").

[joachim, 2015-11-20 23:16:30]

Description was changed from

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (from silviu.cpp@gmail.com).

BUG=webrtc:2030
==========

to

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030
==========

[joachim, 2015-11-25 22:18:00]

On 2015/11/20 23:14:26, joachim wrote:
> As expected, the diff looks rather large, however I only changed a few things:
> - Extracted classes "PhysicalSocket" and "SocketDispatcher" into header file,
so
> "DoAccept" can be overwritten from tests.
> - Converted "NULL" to "nullptr" in code I touched.
> - Added "GUARDED_BY" annotation to attribute in "PhysicalSocket".
> - Added testcase to verify the socket keeps listening after "accept" returned
an
> error (based on "SocketTest::ConnectInternal").

ping?

[pthatcher1, 2015-12-03 22:56:33]

lgtm

But please change "mock" to "fake".

Oh, and sorry the review took so long.  The diff was big :P.

https://codereview.webrtc.org/1452903006/diff/20001/webrtc/base/physicalsocke...
File webrtc/base/physicalsocketserver_unittest.cc (right):

https://codereview.webrtc.org/1452903006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver_unittest.cc:27: class MockSocketDispatcher :
public SocketDispatcher {
I think we would call these Fake, not Mock

[joachim, 2015-12-06 20:02:24]

All renamed (also rebased to some newer master).

[joachim, 2015-12-06 20:02:50]

The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-06 20:02:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452903006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452903006/40001

[commit-bot: I haz the power, 2015-12-06 20:04:50]

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

[commit-bot: I haz the power, 2015-12-06 20:04:50]

Dry run: Try jobs failed on following builders:
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5748)
  mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/5703)
  win_drmemory_light on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)

[joachim, 2015-12-06 20:09:37]

The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-06 20:09:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452903006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452903006/60001

[commit-bot: I haz the power, 2015-12-06 20:11:56]

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

[commit-bot: I haz the power, 2015-12-06 20:11:57]

Dry run: Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/9598)

[joachim, 2015-12-06 20:15:04]

The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-06 20:15:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452903006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452903006/80001

[commit-bot: I haz the power, 2015-12-06 20:17:15]

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

[commit-bot: I haz the power, 2015-12-06 20:17:16]

Dry run: Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9571)
  win_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)

[joachim, 2015-12-06 20:50:00]

Looks like there is more work required to make this work on Windows (i.e.
extract the Windows-specific "SocketDispatcher" class).

I'll let you know when there is more to review.

[joachim, 2015-12-07 21:15:30]

Ptal

Changes since your review:
- The SocketDispatcher class in the header also contains the methods/members of
the Windows version.
- The methods are not implemented twice (one each inside a WEBRTC_WIN and
WEBRTC_POSIX block), but only once with conditional defines in the methods where
necessary.

https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:552: , id_(0), signal_close_(false)
I didn't know how to properly format this according to the styleguide...

https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:560: , id_(0), signal_close_(false)
I didn't know how to properly format this according to the styleguide...

[pthatcher1, 2015-12-08 20:20:43]

I appreciate that you're trying to reduce code duplication, but I have two
concerns:

1.  It made the diff way bigger.  At the very least, I think we should break
this into two CLs:  one two change the behavior and a follow up one to do the
code deduplication.  That would make it much easier to review and understand.

2.  I'm not convinced it makes the code easier to manage.  I think it might make
it harder to read/understand.  But, if it were broken up into 2 separate CLs,
that would be a lot easier to judge.

So, could you please make a version of this CL that doesn't have the dedupe, and
then make a separate CL to do the dedupe?  Otherwise, it's going to take me a
very long time to review this.

https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:552: , id_(0), signal_close_(false)
On 2015/12/07 21:15:30, joachim wrote:
> I didn't know how to properly format this according to the styleguide...

Maybe this?

#if defined(WEBRTC_WIN)
  : PhysicalSocket(ss), id_(0), signal_close_(false)
#else
  : PhysicalSocket(ss)
#endif

[joachim, 2015-12-08 21:58:56]

I understand your concerns and reverted the deduplication from the previous
patch set.

So the only change since your last review is that the SocketDispatched class
inside the #if defined(WEBRTC_WIN) also moved to the .h and its methods are
implementations in the .cc now.

The total diff is still rather large, mostly due to code now being unindented
two spaces because it's no longer inlined in a class in the .cc but an
implementation of the .h file.


On 2015/12/08 20:20:43, pthatcher1 wrote:
> I appreciate that you're trying to reduce code duplication, but I have two
> concerns:
> 
> 1.  It made the diff way bigger.  At the very least, I think we should break
> this into two CLs:  one two change the behavior and a follow up one to do the
> code deduplication.  That would make it much easier to review and understand.
> 
> 2.  I'm not convinced it makes the code easier to manage.  I think it might
make
> it harder to read/understand.  But, if it were broken up into 2 separate CLs,
> that would be a lot easier to judge.
> 
> So, could you please make a version of this CL that doesn't have the dedupe,
and
> then make a separate CL to do the dedupe?  Otherwise, it's going to take me a
> very long time to review this.
> 
>
https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
> File webrtc/base/physicalsocketserver.cc (right):
> 
>
https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
> webrtc/base/physicalsocketserver.cc:552: , id_(0), signal_close_(false)
> On 2015/12/07 21:15:30, joachim wrote:
> > I didn't know how to properly format this according to the styleguide...
> 
> Maybe this?
> 
> #if defined(WEBRTC_WIN)
>   : PhysicalSocket(ss), id_(0), signal_close_(false)
> #else
>   : PhysicalSocket(ss)
> #endif

[joachim, 2015-12-17 22:47:07]

On 2015/12/08 21:58:56, joachim wrote:
> I understand your concerns and reverted the deduplication from the previous
> patch set.
> 
> So the only change since your last review is that the SocketDispatched class
> inside the #if defined(WEBRTC_WIN) also moved to the .h and its methods are
> implementations in the .cc now.
> 
> The total diff is still rather large, mostly due to code now being unindented
> two spaces because it's no longer inlined in a class in the .cc but an
> implementation of the .h file.
> 
> 
> On 2015/12/08 20:20:43, pthatcher1 wrote:
> > I appreciate that you're trying to reduce code duplication, but I have two
> > concerns:
> > 
> > 1.  It made the diff way bigger.  At the very least, I think we should break
> > this into two CLs:  one two change the behavior and a follow up one to do
the
> > code deduplication.  That would make it much easier to review and
understand.
> > 
> > 2.  I'm not convinced it makes the code easier to manage.  I think it might
> make
> > it harder to read/understand.  But, if it were broken up into 2 separate
CLs,
> > that would be a lot easier to judge.
> > 
> > So, could you please make a version of this CL that doesn't have the dedupe,
> and
> > then make a separate CL to do the dedupe?  Otherwise, it's going to take me
a
> > very long time to review this.
> > 
> >
>
https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
> > File webrtc/base/physicalsocketserver.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1452903006/diff/140001/webrtc/base/physicalsock...
> > webrtc/base/physicalsocketserver.cc:552: , id_(0), signal_close_(false)
> > On 2015/12/07 21:15:30, joachim wrote:
> > > I didn't know how to properly format this according to the styleguide...
> > 
> > Maybe this?
> > 
> > #if defined(WEBRTC_WIN)
> >   : PhysicalSocket(ss), id_(0), signal_close_(false)
> > #else
> >   : PhysicalSocket(ss)
> > #endif

Ping, should I assign this to somebody else if you're too busy?

[pthatcher1, 2015-12-17 23:40:49]

lgtm

[pthatcher1, 2015-12-17 23:41:12]

The CQ bit was checked by pthatcher@webrtc.org

[commit-bot: I haz the power, 2015-12-17 23:41:19]

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

[commit-bot: I haz the power, 2015-12-17 23:46:13]

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

[commit-bot: I haz the power, 2015-12-17 23:46:18]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2488)

[joachim, 2015-12-18 00:26:30]

On 2015/12/17 23:40:49, pthatcher1 wrote:
> lgtm

Thanks and sorry for bothering you again on this CL ;-)

The presubmit check complains about lots of "TODO" entries without a name (which
obviously were there before). Any suggestions who I should add there?

[pthatcher1, 2015-12-18 02:30:26]

You can put me: TODO(pthatcher).

And don't apologize for being diligent about getting work done.  If anything, I
should be apologizing for not being faster with review turn around.  Thank you
for the diligence and high-quality work.

[joachim, 2015-12-18 08:45:10]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2015-12-18 08:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1452903006/#ps180001 (title:
"Fixed presubmit")

[commit-bot: I haz the power, 2015-12-18 08:45:13]

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

[joachim, 2015-12-18 08:46:15]

On 2015/12/18 02:30:26, pthatcher1 wrote:
> You can put me: TODO(pthatcher).

Changed, thanks.

> And don't apologize for being diligent about getting work done.  If anything,
I
> should be apologizing for not being faster with review turn around.  Thank you
> for the diligence and high-quality work.

[commit-bot: I haz the power, 2015-12-18 09:39:59]

Description was changed from

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030
==========

to

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030
==========

[commit-bot: I haz the power, 2015-12-18 09:40:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-18 09:40:10]

Description was changed from

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030
==========

to

==========
Keep listening if "accept" returns an invalid socket.

There is an issue in PhysicalSocket::Accept where the flag to continue
listening is not set in "enabled_events_" if "accept" returns an error.
This CL fixes this (initial idea by silviu.cpp@gmail.com).

BUG=webrtc:2030

Committed: https://crrev.com/095ae15d6b9ff60357b44ed6f4997754079eff2e
Cr-Commit-Position: refs/heads/master@{#11080}
==========

[commit-bot: I haz the power, 2015-12-18 09:40:11]

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