Feedback on send/recv functions for non blocking sockets in client side HTTP library

Question

I'm working with TCP sockets in C, specifically only for client side HTTP(S) requests, and would like to get some feedback on my send and recv code.

You can make some assumptions regarding my code, as it is, by no means a complete example but I'll try to make it reproducible.

• Assume sfd has been set to O_NONBLOCK
• Assume SOCKET_ERROR is a macro for -1
• Assume POLLFD is a typedef for struct pollfd
• Assume RESPONSE_BUFFER_LEN is 4096
• Assume errno_is_ok is a macro to check if errno is set to EWOULDBLOCK, EAGAIN or EINTR - these errnos are ignored
• Assume extend_resbuff (used in the recv code) is a function that extends resbuff by multiplying its current len with 2. It takes care of alloc failures by itself
• Assume trim_resbuff (used in the recv code) is a function that trims the resbuff to the exact size it needs to be and null terminates it
• The message sent using my sender function will always contain Connection: close as a header.

My send code, assume a connect call has been made. Also assume connect has returned - or rather, set errno to - EINPROGRESS.

/*
Send given message through given socket
Sends the message in its entirety

Returns true upon success, false upon failure
*/
bool send_all(socket_t sfd, char const* restrict msg, ssize_t msglen)
{
    ssize_t sent = 0;
    ssize_t stat = 0;
    do
    {
        /* Poll for readying the send */
        POLLFD pfds[1] = { { .fd = sfd, .events = POLLOUT } };
        if (poll(pfds, sizeof(pfds) / sizeof(pfds[0]), POLL_TIMEOUT) == 0)
        {
            /* Timeout */
            return false;
        }
        if (pfds[0].revents & POLLOUT)
        {
            /* Ready to write */
            stat = send(sfd, msg + sent, msglen - sent, 0);
            sent += stat;
        }
        else
        {
            /*
            Is it safe to assume an errno is set in this branch?
            The caller is then expected to check the errno

            If this branch is hit, is recovery possible (within the scope
            of this function)?
            */
            return false;
        }
        /*
        This loop exits either when
          * full message is sent
          * stat is SOCKET_ERROR but errno **is not** EAGAIN or EWOULDBLOCK or EINTR
        */
    } while (sent < msglen && (stat != SOCKET_ERROR || errno_is_ok));
    return stat != SOCKET_ERROR;
}

Also of note- the msg is always an HTTP request. Something like GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n. That Connection: close is always present in the headers.

Now, the recv code.

/*
Receive response through given socket
Receives the message in its entirety and stores it into resbuff
resbuff does not need to be allocated - this function manages the allocation

Returns true upon success, false upon failure
*/
bool recv_all(socket_t sfd, char** restrict resbuff, size_t* restrict len)
{
    ssize_t stat = 0;
    size_t idx = 0;                         /* Latest initialized element index of *resbuff */
    *len = RESPONSE_BUFFER_LEN;             /* Length of *resbuff (initially) */
    /* Prepare the resbuff */
    *resbuff = malloc(*len * sizeof(**resbuff));
    if (*resbuff == NULL)
    {
        /* malloc failed */
        return false;
    }
    /* Poll for readying the recv */
    POLLFD pfds[1] = { { .fd = sfd, .events = POLLIN } };
    /* Read responses and append to resbuff until connection is closed */
    do
    {
        if (poll(pfds, sizeof(pfds) / sizeof(pfds[0]), POLL_TIMEOUT) == 0)
        {
            /* Timeout */
            return false;
        }
        /* Extend the buffer if at limit */
        if (idx == *len && !extend_resbuff(resbuff, len))
        {
            /* extend_resbuff failed */
            return false;
        }
        if (pfds[0].revents & POLLIN)
        {
            /* Ready to read */
            stat = recv(sfd, *resbuff + idx, *len - idx, 0);
            idx += (size_t)stat;
        }
        else if (pfds[0].revents & POLLHUP)
        {
            /* Connection closed on remote side - response is most likely all read */
            /*
            I have noticed linux does not reach this even when response is over
            recv, just keeps executing and it keeps returning 0
            which is why the loop exits when recv is 0

            However, on windows (WSAPoll instead of poll) - this branch is triggered
            */
            break;
        }
        else
        {
            /*
            Is it safe to assume an errno is set in this branch?
            The caller is then expected to check the errno

            If this branch is hit, is recovery possible (within the scope
            of this function)?
            */
            return false;
        }
        /*
        This loop exits either when
          * Full response is received and connection is closed (stat is 0)
          * stat is SOCKET_ERROR but errno **is not** EAGAIN or EWOULDBLOCK or EINTR
        */
    } while (stat > 0 && (stat != SOCKET_ERROR || errno_is_ok));
    /*
    Trim resbuff to exactly the size it needs to be (only if stat is not -1)
    the following returns true only if everything succeeds

    (trim_resbuff will not be called if stat is SOCKET_ERROR  in the first place)
    */
    return stat != SOCKET_ERROR && trim_resbuff(resbuff, idx, len);
}

My primary doubts can be seen in form of comments in my code. Also, not necessarily regarding the code in question but are there any socket options I should change that may make these operations more efficient? Options such as TCP_NODELAY, TCP_QUICKACK, SO_RCVBUF, and SO_SNDBUF. Are the default values for these options good enough?

Note: Performance, even microseconds (not milli), is crucial for this specific implementation. Although that does not mean implementing epoll (for linux) and/or an async event loop. I just want the best performance possible using poll and non blocking sockets :)

Answer 1

Overview

I don' think the way you are using poll() is affective. You are basically moving the busy loop from send()/recv() to the poll() function but then giving up when there is a timeout.

If your socket is on loopback that may work great but anything coming across the internet is going to potentially have long waits at some point thus causing your reads to be abandoned and never resumed.

how I would structure it:

  void pollLoop()
  {
       bool finished = false;
       do {    
           int count = poll(/* Very short sleep or use signal to force dropout*/);
           if (count < 0) {
               handleError();
           }
           for(int loop = 0;loop < count; ++loop) {
               handleSocket(loop);
           }
           getNewSocketsThatHaveBeenAdded();
       }
       while(!finished);
   }

   void addSocket(int socket, int type /*read or write */, callback, callbackdata)
   {
        lockGlobalMutexForSocket();
        AddInfoToSo_getNewSocketsThatHaveBeenAdded_PicksItUp();
        unlockGlobalMutex();
        // Optionally create a signal so poll() drops out of sleep
   }

   void getNewSocketsThatHaveBeenAdded()
   {
        lockGlobalMutexForSocket();
        // Add data stored by addSocket to data structure used by poll
        // This may be basically a null op.
        // As long as there is no reallocation the above function can
        // simply append socket information this function will result
        // in the size of the structured used by poll() being larger
        // i.e. parameter 2 in poll() `nfds` increases.
        unlockGlobalMutex();
   }

   void handleSocket(loop)
   {
        // Important.
        // Set the appropriate fd to negative in the poll structure
        // so that poll does not report on this socket while you
        // are handling it.
        fd[loop].fd = -fd[loop].fd; // You flip it back when you are done.

        if (fd[loop].dataAvailable) {
             AddToThreadPool(readOrWriteDataAsAppropriate, loop);
        }
        else /* No data available we have reached the end */
             AddToThreadPool(callSocketCallBackWithData, loop);
        }
 }
 

This the basic for most servers (though I would use libevent personally rather than poll() or ppoll()). With this type of structure a handfull of threads can easily handle 10's of thousands of simultaneous connection.

Code Review

Does C support bool? I though that was C++. I though the C version was slightly different?

bool send_all(socket_t sfd, char const* restrict msg, ssize_t msglen)

---

This must be modern C syntax.
Heard about it not seen it before.

        POLLFD pfds[1] = { { .fd = sfd, .events = POLLOUT } };

---

You nfds is always 1!

        if (poll(pfds, sizeof(pfds) / sizeof(pfds[0]), POLL_TIMEOUT) == 0)

---

Basically you will give up if there is any significant delay. But you don't return any information about how far you got. So it is not possible to resume. If you are going to do it this way then this failure should give you the opportunity to resume by including return data about how far you got.

        {
            /* Timeout */
            return false;
        }

---

You don't check for negative values from poll(). Sometimes there will be an error (or signal) you need to check for these.

---

You don't check for errors on send(). You need to do that.

            stat = send(sfd, msg + sent, msglen - sent, 0);

---

Well it better be a OUT signal since you are sending data. But don't you all expect there at some point to be a response on the same socket? With the current implementation you need to complete the send before you start receiving data. What happens if the server on the other end starts to send data before you have finished sending your data? Not all operations require all the data before they can start responding!

        if (pfds[0].revents & POLLOUT)

---

You should be explicitly checking for an error.

        else
        {
            /*
            Is it safe to assume an errno is set in this branch?
            The caller is then expected to check the errno

            If this branch is hit, is recovery possible (within the scope
            of this function)?
            */
            return false;
        }

---

This is fine.

    } while (sent < msglen && (stat != SOCKET_ERROR || errno_is_ok));

There are several types of error that are not actually errors and you simply try again().