Well, i think that if the whole thing works ok on linux (no segm.faults thrown and the file is written correctly), then the logic must be correct (body buffer must be of the correct size).Quote:
Originally Posted by VladimirF
Dont you think?
Printable View
Well, i think that if the whole thing works ok on linux (no segm.faults thrown and the file is written correctly), then the logic must be correct (body buffer must be of the correct size).Quote:
Originally Posted by VladimirF
Dont you think?
No, I don't agree.Quote:
Originally Posted by hi1
First, your Linux might be more forgiving to memory access violation. For example, it might add more padding.
Second, is it an isolated test? Who fills that "body"? How can you (or rather I) be sure that its content is the same between runs on Linux and Windows? Do you read this data from the file? Was that test file properly transferred cross platforms? Is it even the same size?
What does it cost you to write a simple self-contained test that writes 103429 bytes on Windows?
Here is what I did (and it worked OK):Code:void test()
{
// first i open the file
FILE* request_file = fopen( "R:\\test.dat", "wb" );
if(request_file == NULL)
{
// error
return;
}
// second, try to write my buffer (already filled)
size_t written_bytes = 0;
char* body = new char[103429];
unsigned int content_length(103429);
if( body != NULL )
{
errno=0;
written_bytes = fwrite((body), 1, (size_t)(content_length), request_file);
perror("fwrite");
}
// third, check fwrite return value
if( written_bytes < 0 )
{
// write error
return;
}
else if( written_bytes != (content_length) )
{
// error - partial body received
return;
}
// else fwrite succeed
}
You are right about that.Quote:
Originally Posted by VladimirF
But i expected that even if there was a mismatch between the buffer size and the "content_length" value the write operation should at least start and then throw an error, which it is not the case on Windows.
The fact that nothing is written on disk suggests me that fwrite returns during pre-conditions checking (this made me think about some OS influence...).
Besides, i don't think that fwrite can know the actual buffer size and make an internal check, since it receives just a pointer to the buffer.
The body is read from a network socket a chunk at a time and the whole buffer is reallocated multiple times with "realloc".Quote:
Originally Posted by VladimirF
Thanks for your help.Quote:
Originally Posted by VladimirF
Tomorrow i will run more tests (even with the debugger), and i'll see if i can package a test case.
Right. But it WILL cause access violation trying to copy the stuff that is not there, and will catch the exceprion.Quote:
Originally Posted by hi1
You were right, i get access violation trying to do "puts(parsed_request->body+100000);".Quote:
Originally Posted by VladimirF
So now i have to recheck all the program logic that fills the buffer...
On Windows, the buffer fills up to 29184 bytes (much smaller than the data i send).Code:#define _READ_CHUNK 512
static void _receive_request( void** buffer, int client_connected_socket )
{
int buffer_current_size = 512;
int buffer_current_offset = 0;
int r = recv( client_connected_socket, (*buffer)+buffer_current_offset, _READ_CHUNK, 0 ); // read first chunk
buffer_current_offset += _READ_CHUNK;
while( r==_READ_CHUNK && r!=0 && r!=-1 )
{
// increase buffer size of a chunk and realloc
buffer_current_size += _READ_CHUNK;
*buffer = realloc(*buffer, buffer_current_size);
// read next chunk
r = recv( client_connected_socket, (*buffer)+buffer_current_offset, _READ_CHUNK, 0 );
// increase offset for next read
buffer_current_offset += _READ_CHUNK;
}
}
static void _handle_http_request( int client_connected_socket )
{
void* http_request_buf = calloc( 512, 1 );
_receive_request( &http_request_buf, client_connected_socket );
// ...
}
Great, did I win something? :)Quote:
Originally Posted by hi1
This condition is redundant: if r is 512, you know that it isn’t 0 or -1; if it isn’t 512 – the rest won’t even be tested.Quote:
Originally Posted by hi1
Also, your allocation strategy is not very effective. You realloc (and copy) your buffer 200 times (for that 100K transfer), wasting time and fragmenting your heap. Few suggestions here:
- you can increase allocation unit to at least 4K;
- when you need more room – double existing space instead of simply adding another unit;
- and the winner is: can you send the total size required at the beginning of you transfer? Then you could first read 4 bytes, allocate entire buffer and be done with that.
Almost forgot: you need to add error checking. Your current situation is a BIG argument for that. At the moment, if you didn’t receive the requested 512 bytes, you simply assume that you are done.Quote:
Originally Posted by hi1
So this turned out to be a problem with the socket API, and has nothing to do with fwrite.
I've tried to increase the receive socket buffer size with "setsockopt", but i still receive partial body.
I know that, but usually HTTP requests are small (only headers), so there is no need to realloc the buffer many times.Quote:
Originally Posted by VladimirF
(btw, i've increased _CHUNCK_SIZE to 4096)
I could, but i think this won't solve the problem.Quote:
Originally Posted by VladimirF
That is one fix. The other fix is to make sure the program doesn't crash when something that the program relies on is not set up correctly.Quote:
Originally Posted by hi1
There should be checks in the code for these possible scenarios you're describing.
Regards,
Paul McKenzie
You are right again. I can't rely on that test.Quote:
Originally Posted by VladimirF
This is why i received partial body on Windows.
On Linux the flag "MSG_WAITALL" is available, but this does not seem a reliable solution.
So this is how i've changed the code now.
First, on Windows recv hanged so i've set a socket receive timeout:
Second, i've modified the _receive_request function like this:Code:int socketRecvTimeout = 1000; // 1 sec
setsockopt(server_socket_fd, SOL_SOCKET, SO_RCVTIMEO, &socketRecvTimeout, sizeof(int));
Now everything seems to work nicely both on Windows and Linux.Code:#define _READ_CHUNK 4096
static void _receive_request( void** buffer, int client_connected_socket )
{
int buffer_current_size = 0;
int buffer_current_offset = 0;
int r = _READ_CHUNK_SIZE;
#if defined(WINDOWS) || defined(WIN32) || defined(_WIN32)
while( r!=0 && r!=-1 )
#else
while( r==_READ_CHUNK_SIZE )
#endif
{
buffer_current_size += _READ_CHUNK_SIZE;
*buffer = realloc(*buffer, buffer_current_size);
r = recv( client_connected_socket, (*buffer)+buffer_current_offset, _READ_CHUNK_SIZE, 0 );
buffer_current_offset += r;
}
Do you think this solution is reliable too?
If you have some better solutions for this "receive a chunk at time" thing, you're welcomed to post them!