• Advertisement
Sign in to follow this  

[WinSock TCP] A problem sending/receiving data

This topic is 2301 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hey, for the last week or so I've been having a weird problem sending data...

First of all, the socket is set to nonblocking.

If my server, for example, is doing this:
SendData(Socket, "My data", true);
SendData(Socket, "My other data", true);

Then only the "My data" also the first packet, will be received by the client.
I noticed a workaround, I don't remember how... but I noticed it anyway.
That if I do this:
SendData(Socket, "My data", true);
Sleep(50);
SendData(Socket, "My other data", true);

Then it will work... Of course 50 is just a random number, it depends on how close the packets are sent after eachother.
The reason I don't just send it like:
SendData(Socket, "My data|My other data", true);
or similar, is because I want to be able to do:
SendData(Socket, "My data", true);
if (Bool) {
SendData(Socket, "My other data", true);
}


What happens if I don't use the Sleep(), is that both of the packets will be sent SUCCESSFULLY, but only the first one will be RECEIVED.
I actually get NO sign whatsoever of the second packet, not a single byte.

Here is the SendData function:
bool SendData(SOCKET sdsock, string sddata, bool sdlog)
{
const char *sdtemp = sddata.c_str();
unsigned int Sent = 0;
int Bytes = 0;
while (Sent < sddata.length()) {
Bytes = send(sdsock, sdtemp + Sent, sddata.length() - Sent, NULL);
if (Bytes == SOCKET_ERROR) {
if (sdlog) {
cout << "\nError while trying to send data: " << sdtemp << "\nBytes sent: " << Sent << "\nError: " << WSAGetLastError() << endl;
}
return false;
} else {
Sent += Bytes;
}
}
if (sdlog) {
cout << "\nSuccessfully sent data: " << sdtemp << "\nBytes sent: " << Sent << endl;
}
return true;
}


If you need any more information, then please tell me.
Thanks in advance :)

Edit: Updated the function to Cornstalk's fix. Note that this didn't fix the problem :(

Share this post


Link to post
Share on other sites
Advertisement
While this may not solve your problem, it looks like you've got a bug in your SendData function. When you call[font="Courier New"] Bytes = send(sdsock, sdtemp, sddata.length() - Sent, NULL);[/font] you are doing it almost right, except that you aren't increasing [font="Courier New"]sdtemp[/font]. It should look something more like [font="Courier New"]Bytes = send(sdsock, sdtemp + Sent, sddata.length() - Sent, NULL);[/font] in order to advance the starting point of buffer. Otherwise, you'll just be resending the data from the beginning of the buffer and not from where you left off.

Share this post


Link to post
Share on other sites

Oh, I see. Thank you. :)

While I can't see anything else in your SendData function that looks wrong, can we see the code where you're receiving the data?

Share this post


Link to post
Share on other sites
Sure.
The § is the symbol I just have to tell the function that "This is the end of the packet!".
It may be a bad solution, but it works.

Here's the function:
bool RecvData(SOCKET rdsock, string &rddata, bool rdlog)
{
char rdtemp[BUFFERSIZE + 1];
while (true) {
if (recv(rdsock, rdtemp, BUFFERSIZE, NULL) <= 0) {
if (rdlog) {
if (WSAGetLastError() != 10035) {
cout << "\nError while trying to receive data: " << rdtemp << "\nError: " << WSAGetLastError() << endl;
}
}
return false;
} else {
for (unsigned int i = 0; i < strlen(rdtemp); i++) {
rddata += rdtemp;
if (rdtemp == '§') {
if (rdlog) {
cout << "\nSuccessfully received data: " << rddata << endl;
}
return true;
}
}
if (rdlog) {
cout << "\nEnd of packet not found. Current data: " << rddata << endl;
}
return false;
}
}
}

Share this post


Link to post
Share on other sites
Well, in your OP, '§' isn't at the end of the strings your sending. Assuming it is at the end of the strings you're sending in your real code though, I'd say using '§' as a message delimiter is a bad idea. The reason is because '§' is not an ASCII character, so if you're compiler is using ASCII I'm not sure exactly how it's representing it in memory, but it can cause goofiness when trying to compare if a char equals '§'. Secondly, if your compiler is using UTF-8, '§' is a multi-byte character--that is, it can't be represented with just one byte, so again, testing if a char equals '§' causes goofiness and can't be done trivially. Try using a different character to test for the end of the message, like '\0' or something.

And one more thing: recv will return the number of bytes received. So you should be using the value returned to know how much data got put into your rdtemp buffer, rather than doing strlen (especially because recv doesn't null terminate your buffer, and strlen will be looking for '\0', so technically strlen could, and probably will, return the wrong value).

Share this post


Link to post
Share on other sites
Well, thanks for pointing those issues out.
However, I haven't noticed any goofiness about the § ... It has never caused me any problems, and I've used it for a while... But I'll look into changing it in the future.
I doubt that is the source of this problem though :)

And about the other thing. I don't think I quite get what you mean...

Share this post


Link to post
Share on other sites

And about the other thing. Do you mean the for loop should be like:
for (unsigned int i = 0; i < BytesReceived; i++) {
rddata += rdtemp;
if (rdtemp == '§') {
if (rdlog) {
cout << "\nSuccessfully received data: " << rddata << endl;
}
return true;
}
}


Yes, that's correct, assuming [font="'Courier New"]BytesReceived = recv(rdsock, rdtemp, BUFFERSIZE, NULL)[/font]

Share this post


Link to post
Share on other sites

Okay :)
Thanks for informing me about bugs in the functions, but the problem is still there :(

Sorry to hear the problem's still around. One more thought: you said it works if you Sleep(), but it doesn't if you don't Sleep(). It's possible that both messages get sent and received at the same time. That is, it's possible that the two calls to SendData are processed fast enough that RecvData is receiving both of them. recv doesn't care about how many times you've called send. All recv cares about is if there's data to be read in. If SendData is sending both datagrams quickly enough, which it probably is, recv just sees there's data ready to be received on the wire, even though there are two different datagrams waiting to be read in, recv just sees it as one continuous stream of data.

Hopefully that made a little sense. What you're probably doing is having recv receive both datagrams, but in your parsing of the data, you return after the end of the first datagram, and ignore the second datagram that is in the last part of the buffer. Try printing out how many bytes you receive and how long rddata is. You'll probably see that BytesReceived > rddata.length(), informing you that there is more data to be parsed, but currently you don't parse it.

You'll have to figure out a way to return the first datagram in rddata, but not throw away any extra data in the buffer you haven't parsed yet. I'd post a little coded example but I'm about to eat dinner. Give that a try (printing out BytesReceived and how long rddata is when you reach your delimiting character) and see what you find.

Share this post


Link to post
Share on other sites
Thanks, I bet that is it... Makes sense!
I'll try to make something tomorrow. Now it's like 1:15 AM and I've got school tomorrow, so I better go to bed.

Thanks again!

Share this post


Link to post
Share on other sites
The problem is you're using TCP but treating it as a packet oriented protocol which it isn't.

TCP = stream oriented
UDP = packet oriented

TCP doesn't care if you are sending once, twice or even more often. The receiving end treats the incoming data as a stream of bytes, consuming as much data as is currently available (and fits in the provided buffer).
UDP treats sent data as distinctive packets. You send twice, you receive two packets. Only downside is that UDP packets are not guaranteed to arrive at all, or in order, or duplicated, so you'd have to build your own protocol on top of that to circumvent these shortcomings which is a solved problem as many games and network engines demonstrate.

Back to your problem.
The specific problem happens at:


if (rdtemp == '§') {
...
return true;
}


by returning early you are basically throwing away the rest of the sent data that you might have received.
But don't worry that happens to most beginners of socket programing :wink: There is also good information in the network forums FAQ

Share this post


Link to post
Share on other sites
Yes, that is what I thought.
Thanks for explaining :)

Maybe you can help me fix the RecvData?
I am a bit stuck at the moment... I'm trying to think of something though.

Share this post


Link to post
Share on other sites
I think you need to start over with the code you are working with and restructure it. When you work with TCP, it's far easier to think in layers when working with the data.

The lowest layer is the raw byte stream you send and receive. At this layer all you are worried about are bytes and making sure you send and receive them. What the bytes represent is totally irrelevant; all you care about is making sure they are processed correctly by the system.

The next layer up is your protocol layer. This layer gives meaning (but not context) to specific bytes and determines how data is processed by the system. For example, using "§" as a delimiter defines your protocol. You know messages begin from the beginning of the stream until a "§" is received.

Finally, the message layer is on top. This layer gives context to the data passed using the protocol. In your case, you only have one type of implicit message, text, but you could expand your protocol to support other types of messages as well. For example, add more delimiters that would result in different processing of the data. I.e., let's say you use "[" and "]" to make a section of text that should be capitalized, that'd be part of the protocol while the ability to "bold" text is part of the message itself..

When receiving data, the process is [Raw Bytes] -> [Protocol] -> [Message(s)]. When sending data, the process is reversed: [Message(s)] -> [Protocol] -> [Raw Bytes]. This means your send/recv logic should be generic, protocol agnostic, and completely reusable for any program really.

Since you are working with TCP, and TCP is a stream protocol, you have to make use of buffering. At this point in your learning and programs, you do not have to be worried about the extra overhead from data copies or allocations or anything like that. You just want good solid code that works and you can understand. You will need to buffer all data you receive at the lowest layer and then allow the next layer to process it separately. Once the protocol layer is done processing it, it reconstructs the messages and buffers those for the system to process. When you go to send data, the reverse happens. You buffer a higher level message first, then let the protocol layer break down the messages into byte buffers, then dispatch the buffers to the raw processing layers.

Putting all this together, here's a simple single threaded, one client example that shows the distinctive separation of the layers. Only the important stuff is commented.
[spoiler]
[source]

#include <winsock2.h>
#include <mswsock.h>
#include <windows.h>
#include <stdio.h>
#include <string>
#include <vector>
#include <list>
#include <iterator>
#include <algorithm>

#pragma comment( lib, "ws2_32.lib" )

int main( int argc, char * argv[] )
{
WSADATA wsadata = {0};
int error = 0;

error = WSAStartup( MAKEWORD( 2, 2), &wsadata );
if( error != 0 )
{
printf( "WSAStartup failed with error (%d).\n", error );

return -1;
}

if( LOBYTE( wsadata.wVersion ) != 2 || HIBYTE( wsadata.wVersion ) != 2 )
{
printf( "WSAStartup does not support version 2.2.\n" );

error = WSACleanup();
if( error == SOCKET_ERROR )
{
printf( "WSACleanup failed with error (%d).\n", WSAGetLastError() );
}

return -1;
}

SOCKET listener = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
if( listener == INVALID_SOCKET )
{
printf( "socket failed with error (%d).\n", WSAGetLastError() );

return -1;
}

sockaddr_in localAddress = { 0 };
localAddress.sin_family = AF_INET;
localAddress.sin_addr.s_addr = inet_addr( "127.0.0.1" );
localAddress.sin_port = htons( 7777 );

error = bind( listener, reinterpret_cast< sockaddr * >( &localAddress ), sizeof( localAddress ) );
if( error == SOCKET_ERROR )
{
printf( "bind failed with error (%d).\n", WSAGetLastError() );

closesocket( listener );

error = WSACleanup();
if( error == SOCKET_ERROR )
{
printf( "WSACleanup failed with error (%d).\n", WSAGetLastError() );
return -1;
}
}

error = listen( listener, 1 );
if( error == SOCKET_ERROR )
{
printf( "listen failed with error (%d).\n", WSAGetLastError() );

closesocket( listener );

error = WSACleanup();
if( error == SOCKET_ERROR )
{
printf( "WSACleanup failed with error (%d).\n", WSAGetLastError() );
return -1;
}
}

sockaddr_in remoteAddress = { 0 };
int remoteAddressSize = sizeof( remoteAddress );
SOCKET client = accept( listener, reinterpret_cast< sockaddr * >( &remoteAddress ), &remoteAddressSize );

if( client != INVALID_SOCKET )
{
printf( "Accepting a connection from %s:%i.\n", inet_ntoa( remoteAddress.sin_addr ), ntohs( remoteAddress.sin_port ) );

u_long mode = 1;
error = ioctlsocket( client, FIONBIO, &mode );
if( error == SOCKET_ERROR )
{
printf( "ioctlsocket failed with error (%d).\n", WSAGetLastError() );
}
else
{
std::list<std::string> incomingMessages;
std::list<std::string> outgoingMessages;

std::vector<char> sendWorkspace;

char recvBuffer[8192];
std::vector<char> recvWorkspace;
bool checkRecvWorkspace = false;

// Client welcome message.
outgoingMessages.push_back( "Welcome!\r\n" );

while( true )
{
//--------------// Protocol processing logic (send) //-----------------------//

if( !outgoingMessages.empty() )
{
std::list<std::string>::iterator itr0 = outgoingMessages.begin();
while( itr0 != outgoingMessages.end() )
{
std::string & message = *itr0;
message += '§';
std::copy( message.begin(), message.end(), std::back_inserter( sendWorkspace ) );
++itr0;
}
outgoingMessages.clear();
}

//--------------// Raw data processing logic (send) //-----------------------//

if( !sendWorkspace.empty() )
{
int count = send( client, &sendWorkspace[0], sendWorkspace.size(), 0 );
if( count == SOCKET_ERROR )
{
error = WSAGetLastError();
if( error != WSAEWOULDBLOCK )
{
printf( "send failed with error (%d).\n", error );
break;
}
}
else
{
sendWorkspace.erase( sendWorkspace.begin(), sendWorkspace.begin() + count );
}
}

//--------------// Raw data processing logic //------------------------------//

int count = recv( client, recvBuffer, sizeof( recvBuffer ), 0 );
if( count == 0 )
{
printf( "Disconnected.\n" );
break;
}
else if( count == SOCKET_ERROR )
{
error = WSAGetLastError();
if( error != WSAEWOULDBLOCK )
{
printf( "recv failed with error (%d).\n", error );
break;
}
}
else
{
std::copy( recvBuffer, recvBuffer + count, std::back_inserter( recvWorkspace ) );
checkRecvWorkspace = true;
}

//--------------// Protocol processing logic //------------------------------//

// Since it is possible we get some data that is not complete, we only need to check
// the workspace once it "changes". Otherwise, since we are in non-blocking mode, we
// would be performing the same redundant checks each loop on data we already know
// is incomplete.
if( checkRecvWorkspace )
{
checkRecvWorkspace = false;

// Loop while we have raw data to process. This is so we can extract as many
// messages at once rather than just one at a time per loop.
while( !recvWorkspace.empty() )
{
std::vector<char> message;
for( size_t idx = 0; idx < recvWorkspace.size(); ++idx )
{
if( recvWorkspace[idx] == '§' ) // alt + 167 in console
{
// Extract the message.
std::copy( recvWorkspace.begin(), recvWorkspace.begin() + idx, std::back_inserter( message ) );

// Remove the message and delimiter from the workspace.
recvWorkspace.erase( recvWorkspace.begin(), recvWorkspace.begin() + idx + 1 );

// Do not continue checking.
break;
}
}

// We only need to continue if we actually have a message to process.
if( message.empty() )
{
break;
}

// Make a null terminated string.
message.push_back( '\0' );

// TODO: Verify message data, invalid characters, etc...

// Save the message for processing by the system.
incomingMessages.push_back( std::string( &message[0] ) );
}
}

//--------------// Message processing logic //-------------------------------//

// Check to see if we have any messages to process. I check empty
// to keep scope space clean of extra variables.
if( !incomingMessages.empty() )
{
bool doExit = false;
std::list<std::string>::iterator itr0 = incomingMessages.begin();
while( itr0 != incomingMessages.end() )
{
std::string & message = *itr0;

// Simple command handling example.

if( message == "exit" )
{
doExit = true;
break;
}
else if( message == "hello" )
{
// Note how we simply save the higher level message to the list
// and let the protocol processing logic take care of the rest.
outgoingMessages.push_back( "world!\r\n" );
}
else
{
printf( "Error: Unprocessed message: %s", message.c_str() );
}

++itr0;
}
incomingMessages.clear();

if( doExit )
{
printf( "Client is exiting...\n" );
break;
}
}

// Prevent 100% CPU usage in this example.
Sleep( 1 );
}
}

error = shutdown( client, SD_BOTH );
if( error == SOCKET_ERROR )
{
printf( "shutdown failed with error (%d).\n", WSAGetLastError() );
}

error = closesocket( client );
if( error == SOCKET_ERROR )
{
printf( "closesocket failed with error (%d).\n", WSAGetLastError() );
}
}
else
{
printf( "accept failed with error (%d).\n", WSAGetLastError() );
}

error = closesocket( listener );
if( error == SOCKET_ERROR )
{
printf( "closesocket failed with error (%d).\n", WSAGetLastError() );
}

error = WSACleanup();
if( error == SOCKET_ERROR )
{
printf( "WSACleanup failed with error (%d).\n", WSAGetLastError() );
}

return 0;
}
[/source]
[/spoiler]

In this trivial example, everything is "inline", but when you use this approach, you can wrap everything up into helper functions and classes/structures to keep things organized and support more than one client. Each "context" object will have a socket, a workspace buffer, and message queues. This way, no matter what underlying send/recv mechanisms you use, the message layer remains the same as well as the protocol layer. If you want to change up the protocol some, the other layers aren't affected and so on.

You won't ever "send" or "recv" data directly, only indirectly through buffering. This way, you can properly handle the semantics of the TCP stream as well as gain some flexibility in your system. It takes some getting used to working with TCP and this approach, but in the long run, it helps make your system a lot more manageable compared to the direction you are going right now. Good luck!

Share this post


Link to post
Share on other sites
Thanks for your reply :)

Though, I think I've got a way of making it work now, to handle it like a stream of bytes, which it is :)

I'm going to make a vector and push an element in for every packet that comes in :)
I think I'm going to need a symbol to check for the start of a packet too, and idk what that will be, I may do something else than using symbols at all.... But I'm probably too stupid to do that :)

So if a packet gets merged to be, "&My data§&Other data§", then the RecvData will push each packet into the packets vector :)
So when RecvData returns, I can just go through all the contents in the vector :)

If you think this is a bad way of doing it, then please warn me. But I think this could work without problems :P

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement