Both your TCPSocket::Send and TCPSocket::Recv functions are incorrect and contain bugs.
In Send, you are using uninitailized variables, not returning the actual number of bytes sent, and have implemented subtraction wrong (?!).
In Recv, your loop condition is incorrect, you need to check for a SOCKET_ERROR return value like in send (-1), and you don't really need a loop of any sort.
Please, take the time to review those functions and make a good effort to fix the issues on your own. If you simply post code and ask people to help you fix it, you won't learn anything and will continue to make the same mistakes over and over.
After you fixed your bugs (or at least really tried hard to), this is how simple those functions need to be:
[spoiler]
// Attempts to send 'size' bytes from 'buffer'. The 'buffer' should be at least
// 'size' bytes upon entry and readable. The function returns how many bytes
// were sent. There is no error reporting mechanism built in, so if the number of
// bytes returned does not match the number of bytes requested to be sent, assume
// an error has occurred and obtain the last socket error to know what happened.
// NOTE: This logic is still susceptible to malicious clients that stop receiving so
// larger sends might end up not going through. You can either not try to send the
// entire buffer or add timing code to timeout the send if it is not completed.
int Send( const char * buffer, int size )
{
// TODO: Implement sanity checks for parameters to make sure they are valid
int offset = 0;
// Loop while we have not sent the entire buffer
while( offset < size )
{
int sent = ::send( getSocket(), buffer + offset, size - offset, 0 );
if( sent == SOCKET_ERROR )
{
break;
}
offset += sent;
/*
// TODO: Consider using a platform sleep of the lowest value
// to give the system a chance to catch up since this was a
// partial send. If you do not and are trying to send too much
// to the client, you will loop many, many times and waste
// a lot of useful CPU in the process.
if( offset != size )
{
Sleep(1);
}
*/
}
// Return how many bytes we actually sent
return offset;
}
// Will attempt to receive up to 'size' bytes into 'buffer'. The 'buffer' should be at least
// 'size' bytes upon entry and writable. The function returns the result of recv, so
// SOCKET_ERROR is returned upon an error, 0 is returned upon a disconnect,
// and all other values returned > 0 indicate how many bytes were actually received.
int Recv( char * buffer, int size )
{
// TODO: Implement sanity checks for parameters to make sure they are valid
return ::recv( getSocket(), buffer, size, 0 );
}
Since you are not using
select, you should not try to receive with fixed sizes since that opens your program up to denial of service attacks where you are expecting N bytes but only 0..N - 1 are ever sent. That is why the Recv function will return 0..N requested bytes rather than N and no looping is required. As a result, you have to properly handle the return value in the code that calls the function. Likewise is true of the Send function, you must properly handle the return value and check any error codes where necessary.[/spoiler]
Fixing those two functions still won't fix all your problems though. It looks like you are still using one shared buffer for all communication, so your locks are wrong. Specifically, you should be doing: "Lock -> Operation 1 -> Operation 2 -> Unlock" rather than "Lock -> Operation 1 -> Unlock -> Lock -> Operation 2 -> Unlock". In your current code, you now have a data corruption possibility from the second thread using the buffer and overwriting data from the first or vice versa. Alternatively, you can just setup a local receive buffer in each thread and there is no need to lock then.
You should also keep track of the pointers you allocate via new so you can delete them at the program end or when both connections are done. Right now, you will simply leak resources until the program crashes from an out of memory exception. Also, you are not handling the error codes for ::accept properly either. You need to make sure the error is not on the remote side. Right now, someone could make your program exit simply by starting an accept and then purposefully trigger a
WSAECONNRESET and all of your connections would go poof. Not good! Is there any particular reason you request winsock 1.1 then set the version to 5 in the Initialize function? It's pretty unnecessary.
Finally, and this is the biggest issue of it all, you start threads to handle your connections, but you exit them after the first send/recv that takes place. This means you are only able to support the first send/recv pair between the program and the remote host, which is pretty useless for anything but simple sites that do not support keep-alive. Reread Antheus's post that shows the logic behind this. You loop while the socket is open and then read some data and immediately send it out to the pair. Since you have a unique buffer for each thread in that case, there is nothing for you to lock/synchronize with the mutexs. Once you do that, you will be able to support traffic past the initial send/recv on the connections. Using a lock and a counter inside the SocketPair object, you can also implement a simple reference counter to know when you can delete the object as well so you do not leak memory.