Getting rid of "benign blocking error" log spam.

webrtc/base/physicalsocketserver.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 #include "webrtc/base/physicalsocketserver.h"
@@ skipping 671 matching lines @@
 
 int SocketDispatcher::next_id_ = 0;
 
 #elif defined(WEBRTC_POSIX)
 
 int SocketDispatcher::GetDescriptor() {
   return s_;
 }
 
 bool SocketDispatcher::IsDescriptorClosed() {
+  if (udp_) {
+    // The MSG_PEEK trick doesn't work for UDP, since (at least in some
+    // circumstances) it requires reading an entire UDP packet, which would be
+    // bad for performance here. So, just check whether |s_| has been closed,
+    // which should be sufficient.
+    return s_ == INVALID_SOCKET;
+  }
   // We don't have a reliable way of distinguishing end-of-stream
   // from readability.  So test on each readable call.  Is this
   // inefficient?  Probably.
   char ch;
   ssize_t res = ::recv(s_, &ch, 1, MSG_PEEK);
   if (res > 0) {
     // Data available, so not closed.
     return false;
   } else if (res == 0) {
     // EOF, so closed.
     return true;
   } else {  // error
     switch (errno) {
       // Returned if we've already closed s_.
       case EBADF:
       // Returned during ungraceful peer shutdown.
       case ECONNRESET:
+      // The normal blocking error.
+      case EWOULDBLOCK:
         return true;

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

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.

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

Oops, fixed. 

       default:
         // Assume that all other errors are just blocking errors, meaning the
         // connection is still good but we just can't read from it right now.
         // This should only happen when connecting (and at most once), because
         // in all other cases this function is only called if the file
         // descriptor is already known to be in the readable state. However,
         // it's not necessary a problem if we spuriously interpret a
         // "connection lost"-type error as a blocking error, because typically
         // the next recv() will get EOF, so we'll still eventually notice that
         // the socket is closed.
         LOG_ERR(LS_WARNING) << "Assuming benign blocking error";
         return false;

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

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]

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]

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.

     }
   }
 }
 
 #endif // WEBRTC_POSIX
 
 uint32_t SocketDispatcher::GetRequestedEvents() {
   return enabled_events_;
 }
 
@@ skipping 867 matching lines @@
        break;
     }
   }
 
   // Done
   return true;
 }
 #endif  // WEBRTC_WIN
 
 }  // namespace rtc