Optimize RecvData wrapper

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);
}

-

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_;

};
{
return (recvCode != INVALID_SOCKET && recvCode != SOCKET_ERROR);
}
{
return (recvCode == INVALID_SOCKET ||
(recvCode == SOCKET_ERROR && errno != EAGAIN));
}
std::string Socket::RecvData()
{
{
char singleChar = 0;
ssize_t recvInt = recv(s_, &singleChar, 1, 0);
if (readMore && (singleChar == recordTerminator))
}
}

-
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. –  200_success Apr 15 '14 at 4:17
@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. –  Corbin Apr 15 '14 at 7:07
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. –  Loki Astari Apr 15 '14 at 14:48
I don't like the validRead() or discardBuffer() they don;t add anything to the code and make following the logic harder. –  Loki Astari Apr 15 '14 at 14:50

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().

-

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.

-
1. Change the name. The function returns when it sees a newline, so the correct name is RecvLine or something similar.

2. recv returns -1 on any error. The way your code is structured, it would bail out without testing the errno.

3. On EAGAIN you should not return, but continue. Also you may want to do something more intelligent on other errors, such as EINTR.

4. 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).

-
can you exaplane in code ? –  user3238714 Apr 15 '14 at 3:06
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. –  Loki Astari Apr 15 '14 at 3:30
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. –  vnp Apr 15 '14 at 5:38
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. –  Loki Astari Apr 15 '14 at 15:01