Getting rid of "benign blocking error" log spam.

Getting rid of "benign blocking error" log spam.

For UDP sockets, instead of calling "recv" with "MSG_PEEK" to see if the socket
is alive, just check whether or not it's been closed. It seems that, at least
on some versions of Android, with some specific conditions involving loopback
sockets, calling "recv" with a buffer that's too small for a UDP packet causes
an EFAULT error.

BUG=webrtc:6715
Review-Url: https://codereview.webrtc.org/2678353006
Cr-Commit-Position: refs/heads/master@{#16522}
Committed: https://chromium.googlesource.com/external/webrtc/+/faedf7f942a216b8874ed90947bb1a8b6564af5d

[nisse-webrtc, 2017-02-09 07:37:27]

nisse@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2017-02-09 07:37:28]

Makes sense to me. You might want to also check explicitly for EWOULDBLOCK, and
only log for unexpected errors.
See cl https://codereview.webrtc.org/2639283004/

[Taylor Brandstetter, 2017-02-09 09:04:21]

On 2017/02/09 07:37:28, nisse-webrtc wrote:
> Makes sense to me. You might want to also check explicitly for EWOULDBLOCK,
and
> only log for unexpected errors.
> See cl https://codereview.webrtc.org/2639283004/

Done

[nisse-webrtc, 2017-02-09 09:29:54]

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

https://codereview.webrtc.org/2678353006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:718: return true;
Return value should be true for EBADF and ECONNRESET, but false for EWOULDBLOCK.


And possibly false also for EINTR, to handle the case of an interrupted system
call correctly.

https://codereview.webrtc.org/2678353006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:730: return false;
And I'd suggest returning true here, to close the stream on all other errors.
Then only reason to handle EBADF and ECONNRESET explicitly, rather than handling
them in the default case, is if we want to skip the logging.

[Taylor Brandstetter, 2017-02-09 10:21:43]

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

https://codereview.webrtc.org/2678353006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:718: return true;
On 2017/02/09 09:29:54, nisse-webrtc wrote:
> Return value should be true for EBADF and ECONNRESET, but false for
EWOULDBLOCK.
> 
> 
> And possibly false also for EINTR, to handle the case of an interrupted system
> call correctly.

Oops, fixed.

https://codereview.webrtc.org/2678353006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:730: return false;
On 2017/02/09 09:29:54, nisse-webrtc wrote:
> And I'd suggest returning true here, to close the stream on all other errors.
> Then only reason to handle EBADF and ECONNRESET explicitly, rather than
handling
> them in the default case, is if we want to skip the logging.

Like I mentioned on the other CL, I'm wary about doing this. A false positive
causes the socket to be closed, whereas a false negative just adds some logging.

[nisse-webrtc, 2017-02-09 11:37:02]

lgtm

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

https://codereview.webrtc.org/2678353006/diff/20001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:730: return false;
On 2017/02/09 10:21:43, Taylor Brandstetter wrote:
> On 2017/02/09 09:29:54, nisse-webrtc wrote:
> > And I'd suggest returning true here, to close the stream on all other
errors.
> > Then only reason to handle EBADF and ECONNRESET explicitly, rather than
> handling
> > them in the default case, is if we want to skip the logging.
> 
> Like I mentioned on the other CL, I'm wary about doing this. A false positive
> causes the socket to be closed, whereas a false negative just adds some
logging.

I think it would be good to list explicitly which errors we want to ignore and
why, and then log and return true (i.e., disconnect) on any other errors. But
not essential for this cl.

[Taylor Brandstetter, 2017-02-09 22:09:52]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-02-09 22:10:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-09 23:09:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486678192292100,
"parent_rev": "22e39708c0aa7f2967a85ecb18f1a088c5f8512f", "commit_rev":
"faedf7f942a216b8874ed90947bb1a8b6564af5d"}

[commit-bot: I haz the power, 2017-02-09 23:09:30]

Description was changed from

==========
Getting rid of "benign blocking error" log spam.

For UDP sockets, instead of calling "recv" with "MSG_PEEK" to see if the socket
is alive, just check whether or not it's been closed. It seems that, at least
on some versions of Android, with some specific conditions involving loopback
sockets, calling "recv" with a buffer that's too small for a UDP packet causes
an EFAULT error.

BUG=webrtc:6715
==========

to

==========
Getting rid of "benign blocking error" log spam.

For UDP sockets, instead of calling "recv" with "MSG_PEEK" to see if the socket
is alive, just check whether or not it's been closed. It seems that, at least
on some versions of Android, with some specific conditions involving loopback
sockets, calling "recv" with a buffer that's too small for a UDP packet causes
an EFAULT error.

BUG=webrtc:6715

Review-Url: https://codereview.webrtc.org/2678353006
Cr-Commit-Position: refs/heads/master@{#16522}
Committed:
https://chromium.googlesource.com/external/webrtc/+/faedf7f942a216b8874ed9094...
==========

[commit-bot: I haz the power, 2017-02-09 23:09:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/faedf7f942a216b8874ed9094...

[pthatcher1, 2017-02-10 18:15:29]

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

[pthatcher1, 2017-02-10 18:15:29]

lgtm