From 0cc77bde72e3bb95c4f3f4f121c1a5c9290556cc Mon Sep 17 00:00:00 2001 From: Mike Magruder Date: Thu, 25 Jul 2013 12:25:06 -0700 Subject: [PATCH] Adjust error handling around socket closure for debugger I had previously made the error handling in DebuggerCommand::Receive() too strict, and we'd fail to read available data from the socket when it was closed right after the data was sent. Loosened it, so if there appears to be data present we'll let the thrift layer have a chance to read the data. I also moved the remaining log messages in that function to traces. There are cases where it is nice to see such a log message printed, but those are very rare and have not happened in the wild in the past many months. Log messages here particularly from the signal polling thread make our test output non-deterministic, and there's higher value in having stable tests :) --- hphp/runtime/debugger/debugger_command.cpp | 12 +++++++----- .../debugger/error_bad_cmd_type_receive.php.expectf | 1 - .../debugger/error_bad_cmd_type_send.php.expectf | 1 - .../debugger/error_short_cmd_receive.php.expectf | 1 - .../quick/debugger/error_short_cmd_send.php.expectf | 1 - 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/hphp/runtime/debugger/debugger_command.cpp b/hphp/runtime/debugger/debugger_command.cpp index c77bad524..abb3e08af 100644 --- a/hphp/runtime/debugger/debugger_command.cpp +++ b/hphp/runtime/debugger/debugger_command.cpp @@ -87,8 +87,10 @@ bool DebuggerCommand::Receive(DebuggerThriftBuffer &thrift, TRACE_RB(1, "DebuggerCommand::Receive: error %d\n", errorNumber); return errorNumber != EINTR; // Treat signals as timeouts } - // Any error bits set indicate that we have nothing to read, so fail. - if (fds[0].revents != POLLIN) { + // If we don't have any data to read (POLLIN) then we're done. If we + // do have data we'll attempt to read and decode it below, even if + // there are other error bits set. + if (!(fds[0].revents & POLLIN)) { TRACE_RB(1, "DebuggerCommand::Receive: revents %d\n", fds[0].revents); return true; } @@ -103,7 +105,7 @@ bool DebuggerCommand::Receive(DebuggerThriftBuffer &thrift, // Note: this error case is difficult to test. But, it's exactly the same // as the error noted below. Make sure to keep handling of both of these // errors in sync. - Logger::Error("%s: socket error receiving command", caller); + TRACE_RB(1, "%s: socket error receiving command", caller); return true; } @@ -147,7 +149,7 @@ bool DebuggerCommand::Receive(DebuggerThriftBuffer &thrift, } default: - Logger::Error("%s: received bad cmd type: %d", caller, type); + TRACE_RB(1, "%s: received bad cmd type: %d", caller, type); cmd.reset(); return true; } @@ -155,7 +157,7 @@ bool DebuggerCommand::Receive(DebuggerThriftBuffer &thrift, // Note: this error case is easily tested, and we have a test for it. But // the error case noted above is quite difficult to test. Keep these two // in sync. - Logger::Error("%s: socket error receiving command", caller); + TRACE_RB(1, "%s: socket error receiving command", caller); cmd.reset(); } return true; diff --git a/hphp/test/quick/debugger/error_bad_cmd_type_receive.php.expectf b/hphp/test/quick/debugger/error_bad_cmd_type_receive.php.expectf index 07ccb2172..6c6ae69f1 100644 --- a/hphp/test/quick/debugger/error_bad_cmd_type_receive.php.expectf +++ b/hphp/test/quick/debugger/error_bad_cmd_type_receive.php.expectf @@ -10,6 +10,5 @@ Breakpoint 1 reached on line 3 of %s/error_bad_cmd_type_receive.php internaltesting badcmdtypereceive Executing internal test... -Receive for command: received bad cmd type: 20001 Unable to communicate with server. Server's down? Unable to reconnect to server, exiting. diff --git a/hphp/test/quick/debugger/error_bad_cmd_type_send.php.expectf b/hphp/test/quick/debugger/error_bad_cmd_type_send.php.expectf index 8088c4eb0..0079b8362 100644 --- a/hphp/test/quick/debugger/error_bad_cmd_type_send.php.expectf +++ b/hphp/test/quick/debugger/error_bad_cmd_type_send.php.expectf @@ -10,4 +10,3 @@ Breakpoint 1 reached on line 3 of %s/error_bad_cmd_type_send.php internaltesting badcmdtypesend Executing internal test... -DebuggerProxy::processInterrupt(): received bad cmd type: 20001 diff --git a/hphp/test/quick/debugger/error_short_cmd_receive.php.expectf b/hphp/test/quick/debugger/error_short_cmd_receive.php.expectf index d93db501a..012b67dd7 100644 --- a/hphp/test/quick/debugger/error_short_cmd_receive.php.expectf +++ b/hphp/test/quick/debugger/error_short_cmd_receive.php.expectf @@ -11,6 +11,5 @@ Breakpoint 1 reached on line 3 of %s/error_short_cmd_receive.php internaltesting shortcmdreceive Executing internal test... DebuggerCommand::recv(): a socket error has happened -Receive for command: socket error receiving command Unable to communicate with server. Server's down? Unable to reconnect to server, exiting. diff --git a/hphp/test/quick/debugger/error_short_cmd_send.php.expectf b/hphp/test/quick/debugger/error_short_cmd_send.php.expectf index b8d668a79..397147ef4 100644 --- a/hphp/test/quick/debugger/error_short_cmd_send.php.expectf +++ b/hphp/test/quick/debugger/error_short_cmd_send.php.expectf @@ -11,4 +11,3 @@ Breakpoint 1 reached on line 3 of %s/error_short_cmd_send.php internaltesting shortcmdsend Executing internal test... DebuggerCommand::recv(): a socket error has happened -DebuggerProxy::processInterrupt(): socket error receiving command