0

I'm trying to make a simple heartbeat check from client to server and vice-versa, if connection on either is broken off unexpectedly it prints a message and calls closesocket.

I spent 8 hours on this and it still isn't acceptable to my mentor. Right now I got something that works, but if breakpoint is placed before while loop and connected client is forcefully closed, trying to go past breakpoint causes crash when it should break the loop and write out error.

Server side code:

int main(int argc, char *argv[])
{
    SOCKET s, sa;
    WSAData oWSAData;
    WORD wVersion = 0x0001;
    WSAStartup(wVersion, &oWSAData);
    
    s = socket(AF_INET, SOCK_STREAM, 0);
    sockaddr_in srv_address;
    memset(&srv_address, 0, sizeof(srv_address));
    srv_address.sin_family = AF_INET;
    srv_address.sin_addr.S_un.S_addr = htonl(INADDR_ANY);
    srv_address.sin_port = htons(1099);
    
    bind(s, (sockaddr*) &srv_address, sizeof(srv_address));
    int l = listen(s, 10);
    
    if (l < 0)
        printf("Listen error\n");
    else
    {
        printf("Listen OK. Listening on port %u\n",
               htons(srv_address.sin_port));
        
        sa = accept(s, NULL, NULL);
        
        while (true)
        {
            char buffer[1000];
            
            int nRecvLen = recv(sa, buffer, 999, 0);
            buffer[nRecvLen] = '\0';
            int r = recv(sa, NULL, 0, 0);
            if (r == SOCKET_ERROR && WSAGetLastError() == WSAECONNRESET)
            {
                printf("Konekcija je naglo prekinuta!\n");
                break;
            }
            else
            {
                if (nRecvLen > 0)
                {
                    for (int i = 0; i < nRecvLen; i++)
                    {
                        cout << buffer[i];
                    }
                }
            }
        }
        closesocket(sa);
        closesocket(s);
    }
    
    WSACleanup();
    return 0;
    
}

and client side:

int main()
{
    SOCKET s;
    WSAData oWSAData;
    WORD wVersion = 0x0001;
    WSAStartup(wVersion, &oWSAData);
    
    s = socket(AF_INET, SOCK_STREAM, 0);
    sockaddr_in srv_address;

    memset(&srv_address, 0, sizeof(srv_address));
    srv_address.sin_family = AF_INET;
    srv_address.sin_addr.S_un.S_un_b.s_b1 = xxx;
    srv_address.sin_addr.S_un.S_un_b.s_b2 = xxx;
    srv_address.sin_addr.S_un.S_un_b.s_b3 = x;
    srv_address.sin_addr.S_un.S_un_b.s_b4 = xxx;
    srv_address.sin_port = htons(1099);
    
    int c = connect(s, (sockaddr*) &srv_address, sizeof(srv_address));
    
    if (c < 0)
    {
        printf("Connection error\n");
        cout << (WSAGetLastError());
    }
    else
    {
        string l = "Heartbeat\n";
        int p = l.size();
        char buff[1000];
        strcpy_s(buff, l.c_str());
        printf("Connected\n");
        
        while (true)
        {
            if (send(s, buff, p, 0) > 0)
            {
                Sleep(1000);
            }
            else
            {
                printf("Konekcija je naglo prekinuta\n");
                shutdown(s, SD_BOTH);
                closesocket(s);
                break;
            }
        }
        WSACleanup();
        return 0;
    }
}
user4581301
  • 33,082
  • 7
  • 33
  • 54
D. G.
  • 23
  • 5
  • 1
    ⟼Remember, it's always important, *especially* when learning and asking questions on Stack Overflow, to keep your code as organized as possible. [Consistent indentation](https://en.wikipedia.org/wiki/Indentation_style) helps communicate structure and, importantly, intent, which helps us navigate quickly to the root of the problem without spending a lot of time trying to decode what's going on. – tadman Feb 05 '21 at 22:09
  • 1
    It would also help to remove all these extraneous blank lines that serve no purpose but to make your code harder to read since we have to scroll. – tadman Feb 05 '21 at 22:09
  • 1
    A) Crashes where? Did you check in a debugger? B) Don't be too hard on yourself, you're learning, you're not "stupid". – tadman Feb 05 '21 at 22:10
  • 1
    The most annoying thing about heartbeats is picking them out of the rest of the data in the stream. – user4581301 Feb 05 '21 at 22:10
  • 1
    How are you framing your regular communication vs. heartbeat communication? Can you use TCP keepalive-type features? – tadman Feb 05 '21 at 22:11
  • 1
    You've avoided most of the n00b `recv` mistakes. Good job. One suggestion: `int nRecvLen = recv(sa, buffer, 999, 0);` -> `int nRecvLen = recv(sa, buffer, sizeof(buffer]) - 1, 0);`. This way if `buffer` changes size you only have to change the code in one place. Really sucks if `char buff[1000];` becomes `char buff[128];` somewhere down the line and the `999` is not also changed.. – user4581301 Feb 05 '21 at 22:16
  • @tadman ahh you beat me to it, i was 1minute too late to send exact same changes, sorry about that, im kinda angry at this simple task i can't wrap my head around. All this app should do is heartbeat check, nothing else, no communication outside of it. – D. G. Feb 05 '21 at 22:17
  • 1
    `int r = recv(sa, NULL, 0, 0);` this I don't quite get. Read 0 bytes to nowhere? What is your intent with this line? Never mind. I see what you going for here. Not sure it's kosher. Need to read me some documentation. – user4581301 Feb 05 '21 at 22:18
  • @user4581301 thanks for buffer, this will be useful in my other app, i had buffer overflow there. int r = recv(sa, NULL, 0, 0); this is part of code that kinda works by reading if first message is null / socket error – D. G. Feb 05 '21 at 22:22
  • I can't find a definition of what should happen in POSIX or MSDN documentation. What you're trying here may or may not work. Test it and see, but might I recommend using [`select`](https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-select) for this job? If the socket gets data or errors out `select` returns and you can figure out what to do next safely. You can also easily put a time-out on `select` so you don't hang and wait forever. – user4581301 Feb 05 '21 at 22:29
  • @user4581301 As much as i hate to admit, I don't understand how to use `select` and acording to my mentor it should be done by lisening to returns of send/recv. Thats what confuses me most, didn't I do that already in the code? – D. G. Feb 05 '21 at 22:34
  • 2
    Try and keep it really simple, like send the bytes `HELO` or `BRRR`, something distinctive and fixed-length. When you have arbitrary length fields, then you need delimiters or a size indicator, which gets messy. Don't forget to flush your socket if it's in buffered mode where it won't actually "send" until it receives enough data or is signalled to do so, or set your socket to be "unbuffered". – tadman Feb 05 '21 at 22:34
  • 1
    When debugging network issues, don't be afraid to use `tcpdump` or [Wireshark](https://www.wireshark.org) to observe your actual network traffic and ensure you're doing things correctly. Sometimes you're not sending what you think you're sending, or you're not sending it to *what* you think you're sending it to. If you use a distinctive port number, filtering to just your own traffic is super easy. – tadman Feb 05 '21 at 22:36
  • 1
    Tadman's got the right of it. Start with really simple loops that play ping-pong with a pair of simple messages. client connects, `send`s "ping", waits on `recv` for "pong", then loops around or prints error message. Server `recv`s for "ping", replies with "pong" and then loops back to `recv`. The thing is this is useless unless you have a timeout on the `recv`s so you can catch a connection that was impolitely cut off. Otherwise `recv` will sit and wait effectively forever for the heartbeat and not tell anyone that it hasn't gotten one yet. – user4581301 Feb 05 '21 at 22:53
  • @tadman its always sending string "Heartbeat" to server side, so lenght is always fixed, nothing else gets sent. It works like this, however as mentiond if server has breakpoint before `while` loop and client is forcefully closed by ALT+F4 if server proceeds after breakpoint it just crashes. What's supposed to happen is to break the loop and close sockets. – D. G. Feb 05 '21 at 22:57
  • 1
    If a socket is politely closed, `recv` will return 0. If an error is detected, `recv` will return an error code. Not receiving data is not an error, so if one side of the connection goes away without informing the other side, in other words the socket is not politely closed, `recv` will not stop waiting for data. Solution: `recv` has to time out. If you set the time-out to longer than the required keep-alive reporting period (I usually use three times the keep-alive reporting period) and `recv` times out, no keep-alive message has been received and you can assume the connection has failed. – user4581301 Feb 05 '21 at 23:20
  • 1
    [Here's an answer covering how to make `recv` time out](https://stackoverflow.com/a/30402286/4581301). – user4581301 Feb 05 '21 at 23:22
  • I mean more the string "heartbeat" is somewhat arbitrary unless you stipulate it's going to be precisely 9 bytes which is a kind of wonky number. – tadman Feb 05 '21 at 23:26

0 Answers0