Optimize RecvData wrapper

Question

How can I improve the string RecvData() function?

std::string Socket::RecvData() {
    std::string strBuffer;
    do{
        char buffer;
        int recvInt = recv(s_, &buffer, 1, 0);
        if (recvInt == INVALID_SOCKET)
        {
            return "";
        }
        if (recvInt == SOCKET_ERROR)
        {
            if (errno == EAGAIN) {
                return strBuffer;
            }
            else {
                // not connected anymore
                return "";
            }
        }

        strBuffer += buffer;
        if (buffer == '\n')  return strBuffer;
    } while (true);
}

Keith · Accepted Answer · 2014-04-15 03:29:51Z

3

I'll purely suggest some changes to code structure, but as @user58697 states, you should carefully check the man page to recv and handle each and every error condition.

The first thing to do is to break up the code a bit. Secondly, a lot of people prefer a single return point. The record terminator should be a defined constant.

Capture the concepts of a valid read and the desire to discard buffered data in methods.

Probably matching the record terminator should also really be broken out into a method.

Also recv returns ssize_t.

class Socket
{
public:
    std::string RecvData();
    static const char recordTerminator = '\n';
private:
    int s_;
    bool validRead(ssize_t recvCode);
    bool discardBuffer(ssize_t recvCode);

};
bool Socket::validRead(ssize_t recvCode)
{
    return (recvCode != INVALID_SOCKET && recvCode != SOCKET_ERROR);
}
bool Socket::discardBuffer(ssize_t recvCode)
{
    return (recvCode == INVALID_SOCKET || 
        (recvCode == SOCKET_ERROR && errno != EAGAIN));
}
std::string Socket::RecvData() 
{
    std::string stringRead;
    bool readMore = true;
    while(readMore)
    {
        char singleChar = 0;
        ssize_t recvInt = recv(s_, &singleChar, 1, 0);
        readMore = validRead(recvInt);
        if (readMore)
            stringRead += singleChar;
        else if (discardBuffer(recvInt))
            stringRead = "";
        if (readMore && (singleChar == recordTerminator))
            readMore = false;       
    } 
    return stringRead;
}

answered Apr 15, 2014 at 3:29

Keith

6834 silver badges7 bronze badges

1

\$\begingroup\$ By getting rid of the early returns, you've made the looping behaviour harder to understand. Early returns are generally a good thing, except that it may make Return Value Optimization trickier. \$\endgroup\$

200_success
– 200_success

2014-04-15 04:17:04 +00:00
Commented Apr 15, 2014 at 4:17
1

\$\begingroup\$ @200_success and in C++11 moving basically negates the problem of a potential return copy, though of course you can incur the cost of a move instead of the relatively zero cost of RVO. \$\endgroup\$

Corbin
– Corbin

2014-04-15 07:07:23 +00:00
Commented Apr 15, 2014 at 7:07
1

\$\begingroup\$ Secondly, a lot of people prefer a single return point. I think that i very old school C. C++ is designed so that multiple return points no longer causes an issue. See RAII. \$\endgroup\$

Loki Astari
– Loki Astari

2014-04-15 14:48:00 +00:00
Commented Apr 15, 2014 at 14:48
\$\begingroup\$ I don't like the validRead() or discardBuffer() they don;t add anything to the code and make following the logic harder. \$\endgroup\$

Loki Astari
– Loki Astari

2014-04-15 14:50:29 +00:00
Commented Apr 15, 2014 at 14:50

Add a comment |

200_success · Accepted Answer · 2014-04-15 04:41:32Z

recv() is a system call, which has relatively high overhead. You should avoid calling recv() just to read one byte at a time. Instead, read a reasonable sized buffer (something like the size of an IP packet). Your wrapper object should maintain a buffer of bytes that have been returned by a previous call to recv() but that have not yet been returned to the caller (bytes after a newline character).

In implementing such a buffered interface, you would be forced to use the return value of recv() correctly. recv() returns the number of bytes read, or -1 if an error occurred. The error code is stored in the errno global variable, not the return value of recv().

Community · Accepted Answer · 2017-04-13 12:40:49Z

I was asked if I can explain my recommendations in the code. Here it goes. Take it with a grain of salt.

int Socket::RecvLine(std::string strBuffer)
{
    while (1) {
        char ch;
        switch(rc = recv(s_, &buffer, 1, 0)) {
        case 0:     // End-of-stream, the peer closed connection
            return DONE;
        case 1:     // Got something
            strBuffer += ch;
            if (ch == '\n')
                return OK;
            break;
        case -1:    // Some kind of error; the code below is just for reference.
                    // Don't use it in production.
            if (errno == EINTR || errno == EAGAIN)
                continue;
            return ERROR;
        }
    }
}

As 200_success mentioned, recv is expensive, and you really want to read as much as availabe, which means a secondary buffering is in order of the day. That's next step.

Jamal · Accepted Answer · 2014-12-07 05:26:39Z

2

Change the name. The function returns when it sees a newline, so the correct name is RecvLine or something similar.
recv returns -1 on any error. The way your code is structured, it would bail out without testing the errno.
On EAGAIN you should not return, but continue. Also you may want to do something more intelligent on other errors, such as EINTR.
You must account for the recvInt to become 0, which actually means "not connected anymore".

In any case you should not abandon the data already collected, so return strBuffer anyway, along with some indication of the reason to return. This suggests a different interface to your method, such as int RecvLine(std::string &strBuffer).

edited Dec 7, 2014 at 5:26

Jamal

35.2k13 gold badges134 silver badges238 bronze badges

answered Apr 15, 2014 at 2:36

vnp

58.7k4 gold badges55 silver badges144 bronze badges

1

\$\begingroup\$ can you exaplane in code ? \$\endgroup\$

NinjaDeveloper
– NinjaDeveloper

2014-04-15 03:06:09 +00:00
Commented Apr 15, 2014 at 3:06
3

\$\begingroup\$ Agree with everything apart from the interface. An interface where you have to check for error codes is error prone (which is the problem with a lot of C interfaces). You should have an interface that is hard to misuse by the user. \$\endgroup\$

Loki Astari
– Loki Astari

2014-04-15 03:30:10 +00:00
Commented Apr 15, 2014 at 3:30
\$\begingroup\$ Let's agree to disagree. The callee doesn't have a knowledge of how/why it was called, and if it may terminate for various reason its only option is to report the reason for termination (in this case, reasons are EVERYTHING_FINE, DONE and UNRECOVERABLE_ERROR). Nobody but the caller may decide on what to do next. PS: I am more than happy to discuss problems with C interfaces, which are plenty. \$\endgroup\$

vnp
– vnp

2014-04-15 05:38:43 +00:00
Commented Apr 15, 2014 at 5:38
\$\begingroup\$ Returning error codes is great internally. It allows you to fix bugs there and then and you have to make sure you fix all error codes and account for them. But leaking them across your public API is a bad idea. The problem is that users of your code often forget to check for error states. A better alternative is to throw an exception. This will force application termination if there is an error that is not explicitly handled (which is what you want because if the error is not handled the application is doing something unintended and therefore probably breaking some assumptions. \$\endgroup\$

Loki Astari
– Loki Astari

2014-04-15 15:01:20 +00:00
Commented Apr 15, 2014 at 15:01

Add a comment |

Stack Exchange Network

Optimize RecvData wrapper

4 Answers 4

You must log in to answer this question.

Hot Network Questions

Optimize RecvData wrapper

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions