winsock chat program: problem with I/O

[blahblah13]

Hi

I think this is a approppriate section to post my code in although my problem is related to I/O. But I am not entirely sure that the problem is I/O.

Anyway. I am learning winsock programming and wants to write a very basic chat program.
The program consists of a threaded server application. The server creates new threads for each connected client that it detects.

The client application is just a simple main (no threads) program that gets input from the user and then calls send() to server.

The server receives the message and broadcasts the message to all clients (although this is my idea, I have not implemented it yet).

The problem is that the input from keyboard (client side) is correct, the received data (server side) is wrong. The characters are correct but it seems like the data is divided where the spaces are (seems some how randomly occuring), and also depending on the previous data sent, because for example: I send: ape ape to server. gets ape ape from server and then writes a longer sentece like: ape apes, the server will get ape ape again and s after that.

For reading input I use cin.getline(buff2, strlen(buff2)). getline discards '\n' and append a '\0' character I think. The problem seems like something with the IO but getline works in such a way that it should not be such a problem.

Anyway the code for the server:

// Server.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "Server.h"

using namespace std;

#ifdef _DEBUG
#define new DEBUG_NEW
#endif


// The one and only application object
CWinApp theApp;

using namespace std;

//vector storing all clients
vector<SOCKET> clients;

//methods
UINT serverThread(LPVOID s);
UINT clientThread(LPVOID c);

int _tmain(int argc, TCHAR* argv[], TCHAR* envp[])
{
    int nRetCode = 0;
    SOCKET server = NULL;

    //create server thread
    cout <<"Press ESC to terminate the program" <<endl;
    AfxBeginThread(serverThread, (LPVOID) server);
    while(_getch() != 27);

    closesocket(server);
    WSACleanup();

    return nRetCode;
}

UINT serverThread(LPVOID s)
{
    SOCKET server = (SOCKET) s;
    //initialize winsock
    WSADATA wsaData;
    int iResult = WSAStartup(MAKEWORD(2,2), &wsaData);
    if(iResult != 0)
    {
        cout <<"ERROR: Failed to initialize winsock.\nCode: " <<WSAGetLastError() <<endl;
        return -1;
    }

    //create address info for the server. Meaning filling in structure
    sockaddr_in thisAddr;    //for use in the internet
    thisAddr.sin_family = AF_INET;
    thisAddr.sin_port = htons((USHORT)1234);    //port number is 1234
    thisAddr.sin_addr.s_addr = INADDR_ANY;    //the ip-address of the server is this machine
    server = socket(AF_INET, SOCK_STREAM, 0);    //0 means that the protocol is depending on the given type
    if(server == INVALID_SOCKET)
    {
        cout <<"ERROR: Failed to create server socket." <<endl;
        return -1;
    }//server socket initialization succeeded
    //bind the socket to a port
    if(bind(server, (sockaddr*)&thisAddr, sizeof(thisAddr)) != 0)
    {
        cout <<"ERROR: Failed to bind server socket to port" <<endl;
        return -1;
    }
    //Start listening for client connections
    if(listen(server, 10) != 0)
    {
        cout <<"ERROR: Failed to listen for clients" <<endl;
        return -1;
    }//no error

    SOCKET client;
    sockaddr_in clientAddr;
    int clientAddrLen = sizeof(clientAddr);

    //create a new thread for each new client
    while(true)
    {
        client = accept(server, (struct sockaddr*) &clientAddr, &clientAddrLen);
        clients.push_back(client);
        AfxBeginThread(clientThread, (LPVOID)client);
    }
}

UINT clientThread(LPVOID c)
{
    char buff[512];
    SOCKET client = (SOCKET) c;
    strcpy(buff, "SERVER READY");
    send(client, buff, strlen(buff), 0);
    int iResult;
    //start processing client meesages and send to all other clients participating in the message
    while(true)
    {
        char buff2[512];
        iResult = recv(client, buff2, strlen(buff2), 0);    //get data from client
        buff2[iResult] = '\0';
        cout <<"From client: " <<buff2 <<endl;
        if(iResult == SOCKET_ERROR)
        {
            cout <<"ERROR: Failed to receive data from server." <<endl;
            break;
        }    
        cout <<"Message sent: " <<buff2 <<endl;
        send(client, buff2, strlen(buff2), 0);
    }

    closesocket(client);
    return 1;
} 

The client code:

// Client.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "Client.h"

#ifdef _DEBUG
#define new DEBUG_NEW
#endif


// The one and only application object

CWinApp theApp;

using namespace std;

int _tmain(int argc, TCHAR* argv[], TCHAR* envp[])
{
    char buff[512];
    //hardcoded
    const char *servername = "127.0.0.1";
    
    unsigned int addr;
    struct sockaddr_in serverAddr;
    struct hostent *hp;

    WSADATA wsaData;
    int iResult = WSAStartup(MAKEWORD(2,2), &wsaData);
    if(iResult != 0)
    {
        cout <<"ERROR: Failed to initialize winsock.\nCode: " <<WSAGetLastError() <<endl;
        return -1;
    }
    SOCKET server = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    if(server==INVALID_SOCKET)
        return -1;
    if(inet_addr(servername)==INADDR_NONE)
    {
        hp=gethostbyname(servername);
    }
    else
    {
        addr=inet_addr(servername);
        hp=gethostbyaddr((char*)&addr,sizeof(addr),AF_INET);
    }
    if(hp==NULL)
    {
        closesocket(server);
        return 0;
    }
    serverAddr.sin_addr.s_addr =*((unsigned long*)hp->h_addr);
    serverAddr.sin_port = htons(1234);
    serverAddr.sin_family = AF_INET;
    if(connect(server,(struct sockaddr*)&serverAddr,sizeof(serverAddr)))
    {
        closesocket(server);
        return 0;    
    }

    int bytesRec = recv(server,buff,strlen(buff),0);
    buff[bytesRec]='\0';
    cout <<"DATA FROM SERVER: " <<buff <<endl;

    while(true)
    {
        char buff2[512];
        cin.getline(buff2, 512);
        cout <<"input: " <<buff2 <<endl;
        send(server, buff2, strlen(buff2), 0);
        bytesRec = recv(server, buff2, strlen(buff2), 0);
        if(bytesRec==SOCKET_ERROR)            
            break;
        buff2[bytesRec] = '\0';
        cout <<"Message received: " <<buff2 <<endl;
    }
    closesocket(server);
    WSACleanup();
    return 0;
} 

test inputs:

(client side)

DATA FROM SERVER: SERVER READY
foo bar
input: foo bar
Message received: foo bar
foo bar par
input: foo bar par
Message received foo bar (got the divided message from server)

(server side)

Press ESC to terminate the program
From client: foo bar
Message sent: foo bar
From client: foo bar
Message sent: foo bar (client sent foo bar par)
From client: par (somehow divided)
Message sent: par

Feels somehow that some '\n' is detected by the getline function although it shouldn't happen because getline discards '\n'.

Thanks in before hand for the help!

[j0nas]

Your recv() call in the server is wrong.
Replace
strlen(buff2)
with
sizeof buff2 - 1

[blahblah13]

Oh thanks. It seems to work now.

I should have read the documentation for the method. Me bad

Ok it says that the parameter should be the size in bytes

But why the -1?
Isn't it wrong in the client too, where I use strlen(buff2)?

I don't quite get it. In the examples that I have seen people uses strlen(buff) in the send and recv methods although the docs says the number of bytes to send.
Can it be so that a char is 1 byte? Only reasonable to me.

Seems like sizeof(buff2) makes it works, but isn't it a waste of data to send? Because it will send the size of the buff2 array in bytes amount of data?
strlen should be better, but I don't seem to get it to work.

A little off topic. Just wondering why '\0' is inserted in the position of the number of bytes received. I means the '\0' shouldn't it be in the array already? Meaning we will have 2 terminating character?

[j0nas]

Ok it says that the parameter should be the size in bytes

But why the -1?
Isn't it wrong in the client too, where I use strlen(buff2)?

sizeof buf -1 is due to your logic where you zero-terminate the string using the return value from recv(). Think what would happened if your buffer is 100 bytes and you specify 100 bytes to read, recv() may return 100 and you do buf[100] = '\0'... buffer over-run error !! That's why you should receive no more than the buffer size - 1.

Yes, you're right about the strlen use in the client code too. For recv(), always do sizeof buf -1. For send() you can do strlen() if the buffer is a string (0-terminated).

I don't quite get it. In the examples that I have seen people uses strlen(buff) in the send and recv methods although the docs says the number of bytes to send.
Can it be so that a char is 1 byte? Only reasonable to me.

Yes, a char is always 1 byte. It's incorrect to use strlen() in recv call.

You should also never assume that the whole buffer is sent/recv in one call. Beginners in socket programming almost always do this mistake.

Seems like sizeof(buff2) makes it works, but isn't it a waste of data to send? Because it will send the size of the buff2 array in bytes amount of data?
strlen should be better, but I don't seem to get it to work.

As I said, use strlen in sends and sizeof in recv's

A little off topic. Just wondering why '\0' is inserted in the position of the number of bytes received. I means the '\0' shouldn't it be in the array already? Meaning we will have 2 terminating character?

Count chars and positions and you'll notice how it works. For instance, the data HELLO is 5 bytes... If we do recv() on this data, recv returns 5. To use this data as a string, the null char should be placed at position 6: buffer[5] = '\0'

[blahblah13]

Ok. Thx things are much clearer now

Thx for the help