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.

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.