HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

TCP socket retry mechanism

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sockettcpretrymechanism

Problem

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_iServe

Solution

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.

Code Snippets

else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  bSuccess = false;
  break;
}
else
{
  WRITELOG("Call to socket API 'select' failed inside recv method, error: %d", WSAGetLastError());
  return false;
}
//decide success or failure
if((iRet > 0) && (FD_ISSET(m_hSocket, &fdWrite)))
{
  return true;
}

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

Context

StackExchange Code Review Q#88396, answer score: 5

Revisions (0)

No revisions yet.