Empty buffer after successful recv

Question

I'm writing a server on Windows in C++ and I'm facing a strange behavior using recv().

I wrote this function:

bool readN(SOCKET s, int size, char* buffer){
    fd_set readset;
    struct timeval tv;
    int left, res;
    FD_ZERO(&readset);
    FD_SET(s, &readset);
    left = size;
    std::cout << "-----called readN to read " << size << " byte" << std::endl;
    while (left > 0) {
        tv.tv_sec = MAXWAIT;
        tv.tv_usec = 0;
        res = select(0, &readset, NULL, NULL, &tv);
        if (res > 0) {
            res = recv(s, buffer, left, 0);
            if (res == 0) {//connection closed by client
                return false;
            }

            left -= res;
            std::cout << "\treceived " << res << " left " << left << std::endl;
            if (left != 0) {
                buffer += res;
            }

        }
        else if (res == 0) { //timer expired
            return false;
        }
        else { //socket error
            return false;
        }
    }
    std::cout << "\t" << buffer << std::endl;
    return true;
}

And I call it like this:

std::unique_ptr<char[]> buffer = std::make_unique<char[]>(size_);
if (readN(sck, size_, buffer.get())) {
    std::cout << "----read message----" << std::endl;
    std::cout <<"\t"<< buffer.get()<< std::endl;
}

The problem is that even if recv() returns a positive number, the buffer is still empty. What am I missing?

Answer 1

I see a few problems in your code.

1. you are not resetting the readset variable each time you call select(). select() modifies the variable. For a single-socket case, this is not too bad, but you should get in the habit of resetting the variable each time.

2. you are not checking for errors returned by recv(). You assume any non-graceful-disconnect is success, but that is not always true.

3. at the end of readN() before returning true, you are outputting the buffer parameter to std::cout, however buffer will be pointing at the END of the data, not the BEGINNING, since it was advanced by the reading loop. This is likely where your confusion about an "empty buffer" is coming from. readN() itself should not even be outputting the data at all, since you do that after readN() exits, otherwise you end up with redundant output messages.

4. if readN() returns true, you are passing the final buffer to std::cout using an operator<< that expects a null terminated char string, but your buffer is not guaranteed to be null-terminated.

Try something more like this instead:

bool readN(SOCKET s, int size, char* buffer){
    fd_set readset;
    struct timeval tv;
    int res;
    std::cout << "-----called readN to read " << size << " byte(s)" << std::endl;
    while (size > 0) {
        FD_ZERO(&readset);
        FD_SET(s, &readset);
        tv.tv_sec = MAXWAIT;
        tv.tv_usec = 0;

        res = select(0, &readset, NULL, NULL, &tv);
        if (res > 0) {
            res = recv(s, buffer, size, 0);
            if (res == SOCKET_ERROR) {
                res = WSAGetLastError();
                if (res == WSAEWOULDBLOCK) {
                    continue; //call select() again
                }
                return false; //socket error
            }

            if (res == 0) {
                return false;  //connection closed by client
            }

            buffer += res;
            size -= res;

            std::cout << "\treceived " << res << " byte(s), " << size << " left" << std::endl;
        }

        /*
        else if (res == 0) {
            return false; //timer expired
        }
        else {
            return false; //socket error
        }
        */

        else {
            return false; //timer expired or socket error
        }
    }

    return true;
}

std::unique_ptr<char[]> buffer = std::make_unique<char[]>(size_);
if (readN(sck, size_, buffer.get())) {
    std::cout << "----read message----" << std::endl;
    std::cout << "\t";
    std::cout.write(buffer.get(), size_);
    std::cout << std::endl;
}

With that said, I would suggest an alternative implementation of readN(), depending on whether you are using a blocking or non-blocking socket.

If blocking, use setsockopt(SO_RCVTIMEO) instead of select(). If recv() fails with a timeout, WSAGetLastError() will report WSAETIMEDOUT:

sck = socket(...);

DWORD timeout = MAXWAIT * 1000;
setsockopt(sck, SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout));

bool readN(SOCKET s, int size, char* buffer){
    int res;
    std::cout << "-----called readN to read " << size << " byte(s)" << std::endl;
    while (size > 0) {
        res = recv(s, buffer, size, 0);
        if (res == SOCKET_ERROR) {
            /*
            res = WSAGetLastError();
            if (res == WSAETIMEDOUT) {
                return false; //timer expired
            }
            else {
                return false; //socket error
            }
            */
            return false; //timer expired or socket error
        }

        if (res == 0) {
            return false; //connection closed by client
        }

        buffer += res;
        size -= res;

        std::cout << "\treceived " << res << " byte(s), " << size << " left" << std::endl;
    }

    return true;
}

If non-blocking, don't call select() unless recv() asks you to call it:

bool readN(SOCKET s, int size, char* buffer){
    fd_set readset;
    struct timeval tv;
    int res;
    std::cout << "-----called readN to read " << size << " byte(s)" << std::endl;
    while (size > 0) {
        res = recv(s, buffer, size, 0);
        if (res == SOCKET_ERROR) {
            res = WSAGetLastError();
            if (res != WSAEWOULDBLOCK) {
                return false; //socket error
            }

            FD_ZERO(&readset);
            FD_SET(s, &readset);
            tv.tv_sec = MAXWAIT;
            tv.tv_usec = 0;

            res = select(0, &readset, NULL, NULL, &tv);
            if (res > 0) {
                continue; //call recv() again
            }

            /*
            else if (res == 0) {
                return false; //timer expired
            }
            else {
                return false; //socket error
            }
            */

            return false; //timer expired or socket error
        }

        if (res == 0) {
            return false; //connection closed by client
        }

        buffer += res;
        size -= res;

        std::cout << "\treceived " << res << " byte(s), " << size << " left" << std::endl;
    }

    return true;
}