How to structure worker thread logic for IOCP

Question

I'm creating a client program that communicates with a device connected to my PC via LAN.

A typical communication between my program and the device is as follows:

Program -> Device   1616000D  08 02 00 00 00 21 11 A1 00 01 22 08 00 // Sender sends data (a specific command to the device) to Receiver
Program <- Device   16160002  80 00 // Receiver sends ACK to sender
Program <- Device   16160005  08 20 00 00 00 // Receiver sends command response to sender
Program -> Device   16160002  80 00 // Sender sends ACK to receiver

The last hex number of the first byte sequence indicates the size of data to follow (D = 13 bytes).

My send routine looks like:

bool TcpConnection::SendCommand(const Command& rCommand, const std::vector<BYTE>& rvecCommandOptions)
{
    std::vector<BYTE> vecCommandData;
    m_commandBuilder.BuildCommand(rCommand, rvecCommandOptions, vecCommandData);

    if (vecCommandData.empty())
        return false;

    PerIoData *pPerIoData = new PerIoData;
    if (!pPerIoData)
        return false;

    SecureZeroMemory(&(pPerIoData->m_overlapped), sizeof(WSAOVERLAPPED));

    pPerIoData->m_socket = m_socket.Get();
    pPerIoData->m_overlapped.hEvent = WSACreateEvent();
    pPerIoData->m_vecBuffer.assign(vecCommandData.begin(), vecCommandData.end());
    pPerIoData->m_wsaBuf.buf = (CHAR*)(&(pPerIoData->m_vecBuffer[0]));
    pPerIoData->m_wsaBuf.len = pPerIoData->m_vecBuffer.size();
    pPerIoData->m_dwFlags = 0;
    pPerIoData->m_dwNumberOfBytesSent = 0;
    pPerIoData->m_dwNumberOfBytesToSend = pPerIoData->m_wsaBuf.len;
    pPerIoData->m_operationType = OP_TYPE_SEND;

    if (!m_socket.Send(pPerIoData))
        return false;

    return true;
}

And my worker thread routine looks like:

DWORD WINAPI TcpConnection::WorkerThread(LPVOID lpParameter)
{
    HANDLE hCompletionPort = (HANDLE)lpParameter;
    DWORD dwNumberOfBytesTransferred;
    ULONG ulCompletionKey;
    PerIoData *pPerIoData;

    DWORD dwNumberOfBytesReceived;
    DWORD dwNumberOfBytesSent;
    DWORD dwFlags;

    while (GetQueuedCompletionStatus(hCompletionPort, &dwNumberOfBytesTransferred, &ulCompletionKey, (LPOVERLAPPED*)&pPerIoData, INFINITE)) 
    {
        if (!pPerIoData)
            continue;

        if ((dwNumberOfBytesTransferred == 0) && ((pPerIoData->m_operationType  == OP_TYPE_SEND) || (pPerIoData->m_operationType  == OP_TYPE_RECEIVE)))
        {
            closesocket(pPerIoData->m_socket);
            delete pPerIoData;
            continue;
        }

        if (pPerIoData->m_operationType == OP_TYPE_SEND)
        {
            pPerIoData->m_dwNumberOfBytesSent += dwNumberOfBytesTransferred;
            if (pPerIoData->m_dwNumberOfBytesSent < pPerIoData->m_dwNumberOfBytesToSend)
            {
                pPerIoData->m_wsaBuf.buf = (CHAR*)(&(pPerIoData->m_vecBuffer[pPerIoData->m_dwNumberOfBytesSent]));
                pPerIoData->m_wsaBuf.len = (pPerIoData->m_dwNumberOfBytesToSend - pPerIoData->m_dwNumberOfBytesSent);

                if (WSASend(pPerIoData->m_socket, &(pPerIoData->m_wsaBuf), 1, &dwNumberOfBytesTransferred, 0, &(pPerIoData->m_overlapped), NULL) == 0)
                    continue;

                if (WSAGetLastError() == WSA_IO_PENDING)
                    continue;
            }
            else if (pPerIoData->m_dwNumberOfBytesSent == pPerIoData->m_dwNumberOfBytesToSend)
            {
                delete pPerIoData;
            }

            // Q1. Do I create a new instance of PerIoData here before calling WSARecv() or reuse pPerIoData?

            // QA. If I did do "PerIoData pPerIoData = new PerIoData" here, how do I handle if this momory allocation request has failed?  Should I simply "continue" or "return -1"?

            // QB. Or is this a wrong place to do this memory allocation to achive the typical communication between my program and the device?

            SecureZeroMemory(&(pPerIoData->m_overlapped), sizeof(WSAOVERLAPPED));

            pPerIoData->m_overlapped.hEvent = WSACreateEvent();
            pPerIoData->m_wsaBuf.buf = (CHAR*)(&(pPerIoData->m_vecBuffer[0]));
            pPerIoData->m_wsaBuf.len = pPerIoData->m_vecBuffer.size();
            pPerIoData->m_operationType = OP_TYPE_RECEIVE;

            if (WSARecv(pPerIoData->m_socket, &(pPerIoData->m_wsaBuf), 1, &dwNumberOfBytesReceived, &(pPerIoData->m_dwFlags), &(pPerIoData->m_overlapped), NULL) == 0)
                continue;

            if (WSAGetLastError() == WSA_IO_PENDING)
                continue;
        }
        else if (pPerIoData->m_operationType == OP_TYPE_RECEIVE)
        {
            if ((pPerIoData->m_vecBuffer[0] == 0x16) && (pPerIoData->m_vecBuffer[1] == 0x16))
            {
                // Q2. Do I need to do SecureZeroMemory(&(pPerIoData->m_overlapped), sizeof(WSAOVERLAPPED)); here?

                // Q3. Or do I new PerIoData?

                pPerIoData->m_wsaBuf.buf = (CHAR*)(&(pPerIoData->m_vecBuffer[0]));
                pPerIoData->m_wsaBuf.len = pPerIoData->m_vecBuffer.size();
                pPerIoData->m_operationType = OP_TYPE_RECEIVE;

                // QC. At this point two syn bytes (0x16) are received.  I now need to receive two more bytes of data (000D = 13 bytes) to find out the size of the actual command response data.
                // If I clear my m_vecBuffer here and try to resize its size to two, I get this debug assertion: "vector iterators incompatible" at runtime.  Do you know how I can fix this problem?

                if (WSARecv(pPerIoData->m_socket, &(pPerIoData->m_wsaBuf), 1, &dwNumberOfBytesReceived, &(pPerIoData->m_dwFlags), &(pPerIoData->m_overlapped), NULL) == 0)
                    continue;

                if (WSAGetLastError() == WSA_IO_PENDING)
                    continue;
            }

            // QD. I'm not sure how to structure this if clause for when m_operationType is OP_TYPE_RECEIVE.  I mean how do I distinguish one receive operation for getting two syn bytes from another for getting data size?
            // One way I can think of doing is to create more receive operation types such as OP_TYPE_RECEIVE_DATA_SIZE or OP_TYPE_RECEIVE_DATA?  So you can have something like below.
            // Is this how you would do it?
        }
        //else if (pPerIoData->m_operationType == OP_TYPE_RECEIVE_DATA_SIZE)
        //{
            // Call WSARecv() again to get command response data
        //}
    }

    return 0;
}

Please see my questions in the code above.

Many thanks

Answer 1

1. As the name of your PerIoData type refers to, you need one data structure per incomplete I/O request. A PerIoData structure should persist from the time you initiate asynchronous I/O with WSASend or WSARecv until the time that you retrieve that request's completion packet off of the I/O completion port using GetQueuedCompletionStatus.
2. You should always reinitialize your OVERLAPPED structures when you're about to start a new request.
3. You can re-use the PerIoData structure as long as the I/O request has completed. Given that you've retrieved pPerIoData off the I/O completion port, you may reuse it for subsequent requests. Just make sure that you've reset any applicable fields in that structure so that it is in a state that is appropriate for a new I/O request.

EDIT to answer follow-up questions:

A. I would continue because you want to continue processing I/O events even though you couldn't initiate an additional request. If you don't continue then you won't be able to handle any more I/O completions. Before you continue you might want to call an error handler of some sort.

B. I don't think there's necessarily a "right" or "wrong" place to allocate, but keep in mind that when you allocate your PerIoData there, what you effectively end up doing is repeated allocations and deletes of the same data structure over and over in a loop. When I write code using I/O completion ports, I allocate a pool of my PerIoData equivalent up front and re-use them.

C. I don't have enough context to know the answer. Show your code that does this and the line where the assertion hits and I might be able to help.

D. You could break up your operation type into finer-grained components as you suggested, such as a OP_TYPE_RECEIVE_DATA_SIZE operation. As a warning, reading a couple of bytes on each WSARecv call won't perform as well as you'd like. Winsock calls are expensive; it's a lot of overhead to make a request for a couple of bytes. I'd suggest that you read a larger block of data into your PerIoData buffer in one WSARecv. Then pull your sizing information out of that buffer, then start copying your data out of that buffer. If there's more data arriving than can fit in the buffer, then you can make additional WSARecv calls until you've read the rest in.