TCP socket retry mechanism

Question

I am writing a C++ client application which will send some data to server and wait for its response. Now the protocol is to wait for a specific timeout and then retry for specific times. If all goes wrong, the client will report a communication failure.

I have implemented the whole matter in a non blocking socket operation. I have some doubts on whether my send/receive methods are correct or not.

Below is my code for TCP communication which is written in VC++ 2005 on Windows platform.

Socket parameters building method

bool CTCPCommunication::OpenConnection(bool bRetryConnect)
{
 //create the socket handle and config its paramaters
 m_hSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (m_hSocket == INVALID_SOCKET)
 {
 WRITELOG("Call to API 'socket' failed, error: %d", WSAGetLastError());
 return false;
 }
 //setting socket address
 CT2CA serverIP(m_csServerIP);
 char* pchServerIP = serverIP;
 m_stAddress.sin_family = AF_INET;
 m_stAddress.sin_addr.S_un.S_addr = inet_addr(pchServerIP);
 m_stAddress.sin_port = htons(m_iServerPort);
 //setting socket timeout
 m_stTimeout.tv_sec = SOCK_TIMEOUT_SECONDS;
 m_stTimeout.tv_usec = 0;
 //set socket to non blocking mode
 unsigned long iMode = 1;
 int iResult = ioctlsocket(m_hSocket, FIONBIO, &iMode);
 if (iResult != NO_ERROR)
 {
 WRITELOG("Call to API 'ioctlsocket' failed, error: %d", WSAGetLastError());
 return false;
 }
 bool bSuccess = false;
 //Called for the first time when starting server connection
 if (bRetryConnect == false)
 {
 bSuccess = InitialConnect();
 }
 //For all the other time when client detects a failure in communication and makes a retry
 else
 {
 bSuccess = Connect();
 }
 return bSuccess;
}

Connection building method

bool CTCPCommunication::Connect()
{
 ReportStatus(App_Stat_Connect);
 CT2CA serverIP(m_csServerIP);
 char* pchserverIP = serverIP;
 WRITELOG("Connecting to server %s:%d", pchserverIP, m_iServerPort);
 //try to connect server 
 connect(m_hSocket, (struct sockaddr *)&m_stAddress, sizeof(m_stAddress));
 //check and wait for the socket to be ready with timeout
 fd_set fdWrite;
 FD_ZERO(&fdWrite);
 FD_SET(m_hSocket, &fdWrite);
 int iRet = select(0, NULL, &fdWrite, NULL, &m_stTimeout);
 //decide success or failure
 if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
 {
 return true;
 }
 return false;
}

Reinitiate connection for a retry

bool CTCPCommunication::RetryConnection()
{
 bool bSuccess = CloseConnection();
 if (bSuccess == false)
 {
 ReportStatus(App_Err_Retry);
 WRITELOG("Unabled to attempt retry as existing connection could not be closed, error: %d", WSAGetLastError());
 return bSuccess;
 }
 bSuccess = OpenConnection(true);
 return bSuccess;
}

Upload data to server

bool CTCPCommunication::UploadDataPacket(char* pchSendData, int iSendDataLen, MessageID eSendMessageID, CString csSendPacketGUID)
{
 bool bSuccess = false;
 m_iRetryCount = 0;
 while (m_iRetryCount <= MAX_RETRY)
 {
 // Pushing data packet to socket
 bSuccess = SendSocketData(pchSendData, iSendDataLen);
 if (bSuccess == true)
 {
 // Receive data from socket
 char chRecvBuff[MAX_RECV_LEN+1] = {0};
 bSuccess = ReceiveSocketData(chRecvBuff, MAX_RECV_LEN+1);
 // Verify response packet for proper GUID
 if (bSuccess == true)
 {
 CString csRecvBuff = CString(chRecvBuff);
 bSuccess = ValidateACK(eSendMessageID, csRecvBuff, csSendPacketGUID);
 if (bSuccess == true)
 {
 break;
 }
 }
 }
 if (bSuccess == false)
 {
 RetryConnection();
 m_iRetryCount++;
 if(m_iRetryCount <= MAX_RETRY)
 {
 ReportStatus(App_Stat_Retry, m_iRetryCount);
 WRITELOG("Attempting retry %d", m_iRetryCount);
 }
 }
 }
 return bSuccess;
}

Send socket data

bool CTCPCommunication::SendSocketData(char* pchData, int iBuffLen)
{
 bool bSuccess = true;
 while (iBuffLen > 0)
 {
 //check whether the socket is ready to write data
 fd_set fdWrite;
 FD_ZERO(&fdWrite);
 FD_SET(m_hSocket, &fdWrite);
 int iRet = select(0, NULL, &fdWrite, NULL, &m_stTimeout);
 if ((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
 {
 int iSentLen = send(m_hSocket, pchData, iBuffLen, 0);
 //sending failed due to socket error
 if (iSentLen == SOCKET_ERROR)
 {
 WRITELOG("Call to socket API 'send' failed, error: %d", WSAGetLastError());
 bSuccess = false;
 break;
 }
 pchData += iSentLen;
 iBuffLen -= iSentLen;
 }
 else
 {
 WRITELOG("Call to socket API 'select' failed inside send method, error: %d", WSAGetLastError());
 bSuccess = false;
 break;
 }
 }
 return bSuccess;
}

Receive socket data

bool CTCPCommunication::ReceiveSocketData(char* pchBuff, int iBuffLen)
{
 bool bSuccess = true;
 while ((iBuffLen-1) > 0)
 {
 //check whether the socket is ready to read data
 fd_set fdRead;
 FD_ZERO(&fdRead);
 FD_SET(m_hSocket, &fdRead);
 int iRet = select(0, &fdRead, NULL, NULL, &m_stTimeout);
 if ((iRet > 0) && (FD_ISSET(m_hSocket, &fdRead)))
 {
 int iRcvdLen = recv(m_hSocket, pchBuff, iBuffLen-1, 0);
 //receive failed due to socket error
 if (iRcvdLen <= 0)
 {
 WRITELOG("Call to socket API 'recv' failed, error: %d", WSAGetLastError());
 bSuccess = false;
 break;
 }
 pchBuff += iRcvdLen;
 iBuffLen -= iRcvdLen;
 }
 else
 {
 WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
 bSuccess = false;
 break;
 }
 }
 return bSuccess;
}

Answer 1

First, you should not compare values to boolean literals in an if statement. This is better off written as bSuccess instead of bSuccess == true and !bSuccess instead of bSuccess == false.

Second, you are modifying a local variable right here, which will go out of scope as soon as you leave the method:

else
{
 WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
 bSuccess = false;
 break;
}

Right after this, you break out of your loop and return bSuccess;. This could be written to demonstrate you are immediately returning a fail signal like this:

else
{
 WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
 return false;
}

Third, you have this snippet:

//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
 return true;
}
return false;

This can be written without ifs like this:

return (iRet > 0) && FD_ISSET(m_hSocket, &fdWrite);

The parenthesis around the first statement are not necessary, but might help readability.

• hypheni: Well, regarding your first and second point. Thats a microsoft convention used in most of the cases, so I do follow that. Third and Fourth points are just to make the code more readable in my opinions.
• nhgrif: @hypheni Never compare against boolean literals. Ever. I'd be more okay seeing a GOTO statement than a comparison verse boolean literal.
• hypheni: @nhgrif: Whats the problem? if (var == true) is more readable than if (var) in IDE like Visual studio where you will find true, false in specific color.
• user34073: @hypheni It isn't always about readability, it is also about correctness.
• hypheni: @Hosch250: Well, I agree, comparing with true is redudant. Anyways I will try to get rid of this habbit. Can you / anyone comment more on the socket send/recv part?

Answer 2

Testing that socket is selected for writability is not enough. Losely speaking it only means that send wouldn't block, and after a failed connect it indeed would return immediately (with errno set to ENOTCONN). A canonical way to test for success is to retrieve the completion error with

 int error;
 int len = sizeof(error);
 getsockopt(n_hSocket, SOL_SOCKET, SO_ERROR, &error, &len);

If error is 0 then connection is successful.

• hypheni: So thats for only connect method?
• vnp: @hypheni So far, yes. More to come.