Fix incorrect timeout calculation in GetLine

Attaching a debugger to a process waiting in GetLine would previously cause the
read() to return immediately with EINTR, which we never retried. Now it retries
until the original timeout has expired.

lib/common/GetLine.cpp

@@ -15,8 +15,9 @@
 	#include <unistd.h>
 #endif
 
+#include "autogen_CommonException.h"
+#include "BoxTime.h"
 #include "GetLine.h"
-#include "CommonException.h"
 
 #include "MemLeakFindOn.h"
 
@@ -56,8 +57,8 @@ GetLine::GetLine()
 //		Created: 2011/04/22
 //
 // --------------------------------------------------------------------------
-bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
-	int Timeout)
+bool GetLine::GetLineInternal(std::string &rOutput, bool preprocess,
+	int timeout)
 {
 	// EOF?
 	if(mEOF) {THROW_EXCEPTION(CommonException, GetLineEOF)}
@@ -66,9 +67,12 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 	rOutput = mPendingString;
 	mPendingString.erase();
 
-	bool foundLineEnd = false;
+	box_time_t start_time = GetCurrentBoxTime();
+	box_time_t remaining_time = MilliSecondsToBoxTime(timeout);
+	box_time_t end_time = start_time + remaining_time;
+	bool found_line_end = false;
 
-	while(!foundLineEnd && !mEOF)
+	while(!found_line_end && !mEOF)
 	{
 		// Use any bytes left in the buffer
 		while(mBufferBegin < mBytesInBuffer)
@@ -81,7 +85,7 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 			else if(c == '\n')
 			{
 				// Line end!
-				foundLineEnd = true;
+				found_line_end = true;
 				break;
 			}
 			else
@@ -93,7 +97,7 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 			// Implicit line ending at EOF
 			if(mBufferBegin >= mBytesInBuffer && mPendingEOF)
 			{
-				foundLineEnd = true;
+				found_line_end = true;
 			}
 		}
 
@@ -102,11 +106,36 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 		{
 			THROW_EXCEPTION(CommonException, GetLineTooLarge)
 		}
-
+
+		if(timeout != IOStream::TimeOutInfinite)
+		{
+			// Update remaining time, and if we have run out and not yet found EOL, then
+			// stash what we've read so far, and return false. (If the timeout is infinite,
+			// the only way out is EOL or EOF.)
+			remaining_time = end_time - GetCurrentBoxTime();
+			if(!found_line_end && remaining_time < 0)
+			{
+			       mPendingString = rOutput;
+			       return false;
+			}
+		}
+
 		// Read more in?
-		if(!foundLineEnd && mBufferBegin >= mBytesInBuffer && !mPendingEOF)
+		if(!found_line_end && mBufferBegin >= mBytesInBuffer && !mPendingEOF)
 		{
-			int bytes = ReadMore(Timeout);
+			int64_t read_timeout_ms;
+			if(timeout == IOStream::TimeOutInfinite)
+			{
+				read_timeout_ms = IOStream::TimeOutInfinite;
+			}
+			else
+			{
+				// We should have exited above, if remaining_time < 0.
+				ASSERT(remaining_time >= 0);
+				read_timeout_ms = BoxTimeToMilliSeconds(remaining_time);
+			}
+
+			int bytes = ReadMore(read_timeout_ms);
 
 			// Error?
 			if(bytes == -1)
@@ -117,7 +146,7 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 			// Adjust buffer info
 			mBytesInBuffer = bytes;
 			mBufferBegin = 0;
-			
+
 			// No data returned?
 			if(bytes == 0 && IsStreamDataLeft())
 			{
@@ -136,7 +165,7 @@ bool GetLine::GetLineInternal(std::string &rOutput, bool Preprocess,
 		}
 	}
 
-	if(Preprocess)
+	if(preprocess)
 	{
 		// Check for comment char, but char before must be whitespace
 		// end points to a gap between characters, may equal start if

test/basicserver/testbasicserver.cpp

@@ -33,8 +33,8 @@
 #define SERVER_LISTEN_PORT	2003
 
 // in ms
-#define COMMS_READ_TIMEOUT					4
-#define COMMS_SERVER_WAIT_BEFORE_REPLYING	40
+#define COMMS_READ_TIMEOUT	4
+#define COMMS_SERVER_WAIT_BEFORE_REPLYING	1000
 // Use a longer timeout to give Srv2TestConversations time to write 20 MB to each of
 // three child processes before starting to read it back again, without the children
 // timing out and aborting.
@@ -67,9 +67,10 @@ void testservers_pause_before_reply()
 #ifdef WIN32
 	Sleep(COMMS_SERVER_WAIT_BEFORE_REPLYING);
 #else
+	int64_t nsec = COMMS_SERVER_WAIT_BEFORE_REPLYING * 1000LL * 1000;	// convert to ns
 	struct timespec t;
-	t.tv_sec = 0;
-	t.tv_nsec = COMMS_SERVER_WAIT_BEFORE_REPLYING * 1000 * 1000;	// convert to ns
+	t.tv_sec = nsec / NANO_SEC_IN_SEC;
+	t.tv_nsec = nsec % NANO_SEC_IN_SEC;
 	::nanosleep(&t, NULL);
 #endif
 }
@@ -91,7 +92,11 @@ void testservers_connection(SocketStream &rStream)
 		std::string line1("CONNECTED:");
 		line1 += rtls.GetPeerCommonName();
 		line1 += '\n';
+
+		// Reply after a short delay, to allow the client to test timing out
+		// in GetLine():
 		testservers_pause_before_reply();
+
 		rStream.Write(line1.c_str(), line1.size());
 	}
 
@@ -313,7 +318,6 @@ void Srv2TestConversations(const std::vector<IOStream *> &conns)
 	{
 		getline[c] = new IOStreamGetLine(*conns[c]);
 
-		bool hadTimeout = false;
 		if(typeid(*conns[c]) == typeid(SocketStreamTLS))
 		{
 			SocketStreamTLS *ptls = (SocketStreamTLS *)conns[c];
@@ -323,10 +327,18 @@ void Srv2TestConversations(const std::vector<IOStream *> &conns)
 			conns[c]->Write("Hello\n", 6);
 
 			std::string line1;
-			while(!getline[c]->GetLine(line1, false, COMMS_READ_TIMEOUT))
-				hadTimeout = true;
-			TEST_THAT(line1 == "CONNECTED:CLIENT");
-			TEST_THAT(hadTimeout)
+
+			// First read should timeout, while server sleeps in
+			// testservers_pause_before_reply():
+			TEST_THAT(!getline[c]->GetLine(line1, false,
+				COMMS_SERVER_WAIT_BEFORE_REPLYING * 0.5));
+			TEST_EQUAL(line1, "");
+
+			// Second read should not timeout, because we should have waited
+			// COMMS_SERVER_WAIT_BEFORE_REPLYING * 1.5
+			TEST_THAT(getline[c]->GetLine(line1, false,
+				COMMS_SERVER_WAIT_BEFORE_REPLYING));
+			TEST_EQUAL(line1, "CONNECTED:CLIENT");
 		}
 	}