Hello to everyone !!

I am using winsock in an application to communicate with some services in a request-response manner but multiple requests can happen at the same time in the same socket because it is kind of a complex system having a loadbalancer and a relay server sitting in the middle i will paste here only the code that I'm not sure that will do the job its written in c.

Im using 6 byte in order to set the length of the message then in a second recv i read until that length is reached.

This is a typical message i am using a delimiter of some kind at the end too for the message processing also the message is base64 and url encoded

000271Rg2hPgN17awwZJrrd5jm/CXWd2979rh4MV5w4yxGoBXQ4BPqLutM5VwE4gWLPA2F64Wc43Tks2eymzV9E3MOp23izNtnFoPmz8wr/OAvFC2OPE+cImMj8fxo9rL6o0K1Pa01kOHgccVE3Bgv4odOHpB1vcQCkDX/Y/ltlCdzw+XbTxZ47LkJdVB8LVAUOCqepHfeSMIuQvTHQh2jwWnuND/mG5OtYGZGgBUpNHopkkT61L4y588QPDcCUi/dxRq141aa5AqcG2o=[^]

I would like to ask you the following

1. Do you think this is sufficient for the task to receive reliable all the messages.
2. Can i improve the code or fix any problematic part.
3. In general suggestions to take it further

I had many problems with missing data but at this state it looks stable with the little testing that i did with 2 clients together asking data also this code run in a thread and I'm opening a thread like that.

HANDLE hHandle = CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)Th,NULL,NULL,NULL);

Thanks a lot for your time !!!!!!!!!!!

	fstream afile;
	int HEADER_SIZE=6;
	int sizeFirstBuffer = HEADER_SIZE*sizeof(char);
	int finalResult = 0;
	char* RCV_BUFFER_SECOND;
	bool isRunning = true;
	while (isRunning) {


		char* RCV_BUFFER=(char*) malloc(sizeFirstBuffer+1);

	
		int result = 0;  

		for(int i=0;i<HEADER_SIZE;i++){
			RCV_BUFFER[i]=' ';
		}
		RCV_BUFFER[sizeFirstBuffer]='\0';
		int sizeThatIHaveToRead= 0;
		while(sizeThatIHaveToRead < 30 || result < sizeFirstBuffer){
 
		 
			
		
			result += recv(tcp_socket,RCV_BUFFER,sizeFirstBuffer - result,0);
			handleError();
		 
			if (result < 0){

	 

				free(RCV_BUFFER);
				free(RCV_BUFFER_SECOND);
				isRunning = false;
				break;

			}

			sizeThatIHaveToRead = atoi(RCV_BUFFER);
			
		}
	
		free(RCV_BUFFER);
		if( sizeThatIHaveToRead == 0) {
			
		
			free(RCV_BUFFER_SECOND);
			continue;	
		}
		
		afile.open(outpath,  fstream::in | fstream::out | fstream::app );
		afile << "" << endl;
		afile.close();
		int sizeSecondBuffer = sizeThatIHaveToRead*sizeof(char);
		RCV_BUFFER_SECOND=(char*) malloc(sizeSecondBuffer+1);



		for(int i=0;i<sizeSecondBuffer;i++){
			RCV_BUFFER_SECOND[i]=' ';
		}


		RCV_BUFFER_SECOND[sizeSecondBuffer]='\0';
		finalResult = 0;
		while((sizeSecondBuffer - finalResult) > 0 ){
			

			finalResult += recv(tcp_socket,RCV_BUFFER_SECOND,sizeSecondBuffer - finalResult,0);
			handleError();

		 
			 
		}

		sizeThatIHaveToRead = 0;
 
		PostMessage(hWndMain,WM_USER+8,sizeSecondBuffer,(LPARAM)RCV_BUFFER_SECOND);
		/*if(reconnects>1){
			Sleep(100);
			
			free(RCV_BUFFER_SECOND);
		}*/
		 

 
	}

C or C++? Seems to be very C style, if using C++, really use RAII (std::unique_ptr etc.), std::string, std::vector, etc. for this.

You can send binary with sockets. Although of course text works and there are many text protocols, in this case it appears to be a lot of extra serialising and parsing effort, but it still doesn't have the advantages of being easily readable since it is encoded.

The capitalisation of RCV_BUFFER and RCV_BUFFER_SECOND is odd and doesn't seem to follow your other conventions. Generally upper case is macros, constants, etc. in most styles. Also RCV_BUFFER is so small and fixed size so it might as well be a stack array.

Not sure why you have both a delimiter and length prefix, but the posted code doesn't seem to use the delimiter. Delimiters are fine, many protocols use them (without a length prefix), general rules are make sure the delimiter value can not appear in the data through some sort of encoding (base64 could do that, although it also loses a lot of advantages to a text based protocol), and if you read too much, need to handle keeping the overflow for next time (might read 4KB pages, then search for delimiters, and likely you will have some bytes left over to put towards the next one).

		while(sizeThatIHaveToRead < 30 || result < sizeFirstBuffer){
 
		 
			
		
			result += recv(tcp_socket,RCV_BUFFER,sizeFirstBuffer - result,0);
			handleError();
		 
			if (result < 0){

	 

				free(RCV_BUFFER);
				free(RCV_BUFFER_SECOND);
				isRunning = false;
				break;

			}

			sizeThatIHaveToRead = atoi(RCV_BUFFER);
			
		}

What is sizeThatIHaveToRead < 30 ? Make sure this can't desync and break your protocol. If its the minimum valid message how does just retrying recover if it got messed up?

You should check the result of recv to handle errors. You are just adding the result to another value in a loop, but if that value is already non-zero (2nd or later iteration) then you can miss the error.You should check for the 0 return not just negative.

I assume handleError() is either aborting or jumping (setjmp/longjmp since this appears to be C). If it is jumping out and continuing you are leaking memory every time it happens.

free(RCV_BUFFER_SECOND) you never assigned it a value before here on the first iterations, so its going to try and free some random address and probably break stuff. Certainly undefined behaviour.

That break statement doesn't exit the outer loop and isRunning is not checked immediately after this loop. So if this fails, and sizeThatIHaveToRead is zero, it will PostMessage a zero byte buffer, not sure that is intended.

If recv was to returns less than the expected 6 bytes (generally unlikely but possible depending on how the sending side got combined and split into packets) you are calling atoi on uninitialised bytes. Now it is null terminated so it can't break too badly, but is still undefined and your going to get some unexpected numbers back, which might make the while loop exit and break everything.

		while((sizeSecondBuffer - finalResult) > 0 ){
			

			finalResult += recv(tcp_socket,RCV_BUFFER_SECOND,sizeSecondBuffer - finalResult,0);
			handleError();

		 
			 
		}

Similar issues, make sure check the return value of recv incase got disconnected.

PostMessage(hWndMain,WM_USER+8,sizeSecondBuffer,(LPARAM)RCV_BUFFER_SECOND);

Not sure on how reliable memory management can be with PostMessage, but probably want to check the return value. I assume your window procedure does free the memory? Consider using SendMessage and freeing the memory after SendMessage returns.

It is written in C. About those two variables, my bad for some reason forgot to correct them thanks a lot for mentioning and for the info.

My first attempt was just to use Winsock as a relay of the data having the delimiter at the end but that didn't work.

I was losing data because I did the mistake to think in the manner of messages when it is about a continuous stream I suspect it has something to do with the overflowing then I used a length and things went smoother since then with the second recv that it's dynamically allocated based on the contents of the parsed first buffer.

SyncViews said:
(might read 4KB pages, then search for delimiters, and likely you will have some bytes left over to put towards the next one)

The 30 is the minimum valid message. I understand where I did wrong didn't account for a problem to arise in the next iterations and only for the first time.

About the retrying to recover the message, I thought that if recv returns something other than a readable chunk length then it will be an error.

I am breaking the inner loop since in case of an error atoi(RCV_BUFFER) will always return 0, so if it's zero it will cancel the second recv won't happen and the loop will continue to find the next valid message.

Then even if a message is delivered through PostMessage I'm looking for a final delimiter so it will not have an effect but of course, I understand where is one possible memory problem.

SyncViews said:
you are calling atoi on uninitialised bytes

I think atoi returns 0 on not valid int string content or characters so in the loop result < sizeFirstBuffer ensures that even if I get a fragment of the header let's say the 3 bytes out of 6 it will keep inside the loop until I get the full header and then atoi again( that part I could do outside of the loop I guess when it passed the requirements of the while)

SyncViews said:
return value of recv incase got disconnected.

About the disconnect, if a user disconnects let's say in a mobile device that is changing cells I can receive the rest of the stream even though the client might have a different IP, and what is a best practice there?

Do you think with the current code I might have a possible message loss?

Thanks again you are helping me very much!

At the higher level, I would give a few pointers (SyncViews already pointed out the obvious bugs that I saw too.):

1. You do not need to marshhal to text. TCP is not like email. Using binary values is perfectly fine, and often preferrable. The easiest way to marshal is to have a “type, size, data” packet format, where “type” and “size” may be fixed size, and “data” then is the number of bytes suggested by “size.”
2. You may want to use some kind of pre-existing mashaling system to generate your streams of bytes (similar to how you do for files on disk, really.) Anything from Google Protocol Buffers to boost serialization to your own simple “append X bytes at a time to a buffer” mechanism could work, but it's important to treat it as a subsystem of its own, so you can debug it once and then know it works.
3. If the maximum packet size is known ahead of time, you can allocate a single buffer that's that size. You don't necessarily need a separate buffer for “header” versus “data," especially if you treat a packet as something a little more structured, like a struct containing fields for “base pointer, allocated size, current read offset, current end offset”
4. A TCP socket handler generally will be a state machine that reads into a buffer, and then decodes as much as it can out of that buffer. Often, this means using a pre-allocated cyclic buffer per socket, and then copying out of this buffer into the actually decoded data fragments. Compared to the overhead of networking, the overhead of copying the data is negligible for most games.
5. The TCP socket receiver will generally “emit events” rather than be a thing that you call into synchronously.