7
\$\begingroup\$

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;
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – hypheni Apr 29 '15 at 20:49
  • 4
    \$\begingroup\$ @hypheni Never compare against boolean literals. Ever. I'd be more okay seeing a GOTO statement than a comparison verse boolean literal. \$\endgroup\$ – nhgrif Apr 29 '15 at 20:54
  • \$\begingroup\$ @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. \$\endgroup\$ – hypheni Apr 29 '15 at 20:59
  • \$\begingroup\$ @hypheni It isn't always about readability, it is also about correctness. \$\endgroup\$ – Hosch250 Apr 29 '15 at 21:00
  • \$\begingroup\$ @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? \$\endgroup\$ – hypheni Apr 29 '15 at 21:06
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ So thats for only connect method? \$\endgroup\$ – hypheni Apr 29 '15 at 21:24
  • \$\begingroup\$ @hypheni So far, yes. More to come. \$\endgroup\$ – vnp Apr 29 '15 at 21:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.