Partial output was caused by failing to read the HTTP request. This patch reads the HTTP request into a temporary buffer before discarding it.
Signed-off-by: Jonathan Mzengeza jtmze87@gmail.com Index: vis.c =================================================================== --- vis.c (revision 1343) +++ vis.c (working copy) @@ -566,6 +566,7 @@ buffer_t *last_send = NULL; size_t ret; char* send_buffer = NULL; + char tmp[512];
while ( !is_aborted() ) {
@@ -579,6 +580,11 @@ send_buffer = current->dot_buffer; } else { send_buffer = current->json_buffer; + ret = read( thread_data->socket, tmp, sizeof( tmp )); + while ( ret == -1 ) { + ret = read( thread_data->socket, tmp, sizeof( tmp )); + usleep(250); + } }
ret = write( thread_data->socket, send_buffer, strlen( send_buffer ) );
Partial output was caused by failing to read the HTTP request. This patch reads the HTTP request into a temporary buffer before discarding it.
This patch creates an endless loop on unrecoverable socket errors. See read(3) for more information about return codes. Please provide more information if I am wrong.
Best regards, Sven
2009/7/15 Sven Eckelmann sven.eckelmann@gmx.de:
Partial output was caused by failing to read the HTTP request. This patch reads the HTTP request into a temporary buffer before discarding it.
This patch creates an endless loop on unrecoverable socket errors. See read(3) for more information about return codes. Please provide more information if I am wrong.
Best regards, Sven
Thanks, is this better?
Signed-off-by: Jonathan Mzengeza jtmze87@gmail.com Index: vis.c =================================================================== --- vis.c (revision 1343) +++ vis.c (working copy) @@ -566,6 +566,7 @@ buffer_t *last_send = NULL; size_t ret; char* send_buffer = NULL; + char tmp[512];
while ( !is_aborted() ) {
@@ -579,6 +580,11 @@ send_buffer = current->dot_buffer; } else { send_buffer = current->json_buffer; + ret = read( thread_data->socket, tmp, sizeof( tmp )); + while ( ret == -1 && errno == EAGAIN) { + ret = read( thread_data->socket, tmp, sizeof( tmp )); + usleep(250); + } }
ret = write( thread_data->socket, send_buffer, strlen( send_buffer ) );
Thanks, is this better?
There are different things I don't understand complete. How does the receive buffer affect the output buffer in this case? Isn't it possible to disable non-blocking sockets in vis.c:635-638 and change the read to following for json: Try to recv data until the read fails or an empty line (please check HTTP rfc before implementing) appears. If the read fails -> discard everything. If the empty line appeared then send the data. You should create an extra function to read (and discard) the header stuff.
My personal opinion about the errno and delay stuff is... I don't like it. It seems to be somewhat correct to ask again on EAGAIN corresponding to the man page, but we could do it in a cleaner way if possible.
Maybe someone sees a problem in my proposal. The only thing I see is that the read should appear before locking the current buffer or a "bad person" (me) could delay the new buffers for all others by connecting and then wait for a long time. I see the same problem in your current implementation.
Regards, Sven
If I can chime in with my 5 cents,
On 15 Jul 2009, at 20:50 , Sven Eckelmann wrote:
Thanks, is this better?
There are different things I don't understand complete. How does the receive buffer affect the output buffer in this case?
Without spending a day rooting through the syscall implementation I'm not sure why this is the case. Neither the documentation nor Google has been much help beyond revealing that:
* It is a widespread phenomena where writes are getting truncated when there is unread data on the socket.
* There exists many opinions and precious little consensus on the precise semantics of read(2) and write(2) !
Oy vey.
Isn't it possible to disable non-blocking sockets in vis.c:635-638 and change the read to following for json: Try to recv data until the read fails or an empty line (please check HTTP rfc before implementing) appears. If the read fails -> discard everything. If the empty line appeared then send the data. You should create an extra function to read (and discard) the header stuff.
If everyone else thinks this is a good idea I would also greatly prefer to modify the vis server to use blocking sockets.
Given that it is a threaded implementation it is not immediately obvious to me why this approach was chosen originally unless the threading got added later?
Thoughts anyone?
My personal opinion about the errno and delay stuff is... I don't like it. It seems to be somewhat correct to ask again on EAGAIN corresponding to the man page, but we could do it in a cleaner way if possible.
*nod*
It _is_ ugly. It is simulating blocking behavior on a non-blocking object which, really, means that one should ideally just be using a blocking object in the first place!
:-D
Maybe someone sees a problem in my proposal. The only thing I see is that the read should appear before locking the current buffer or a "bad person" (me) could delay the new buffers for all others by connecting and then wait for a long time. I see the same problem in your current implementation.
I'm not sure the current implementation performs a read between locks but I agree that the server should not block other connections while waiting for a response from another connection.
Thank you for the code review Sven!
- antoine
-- http://7degrees.co.za "Libré software for human education."
Maybe someone sees a problem in my proposal. The only thing I see is that the read should appear before locking the current buffer or a "bad person" (me) could delay the new buffers for all others by connecting and then wait for a long time. I see the same problem in your current implementation.
I'm not sure the current implementation performs a read between locks but I agree that the server should not block other connections while waiting for a response from another connection.
I didn't wanted to say that the current implementation in trunk uses a read, but Jonathan Mzengeza's patch does it.
Best Regards, Sven
On 16 Jul 2009, at 09:36 , Sven Eckelmann wrote:
Maybe someone sees a problem in my proposal. The only thing I see is that the read should appear before locking the current buffer or a "bad person" (me) could delay the new buffers for all others by connecting and then wait for a long time. I see the same problem in your current implementation.
I'm not sure the current implementation performs a read between locks but I agree that the server should not block other connections while waiting for a response from another connection.
I didn't wanted to say that the current implementation in trunk uses a read, but Jonathan Mzengeza's patch does it.
It _is_ Jonathan's patch I'm referring to :-)
As it stands his code does not delay the new buffers for all others if someone connects and then waits a long time.
But that is a choopchick and not really important one way or another, I think we both agree that switching to blocking sockets is a promising idea.
Does anyone with insights into the gooey innards of vis.c have any thoughts about this strategy?
- a
On Thursday 16 July 2009 15:45:19 Antoine van Gelder wrote:
But that is a choopchick and not really important one way or another, I think we both agree that switching to blocking sockets is a promising idea.
Does anyone with insights into the gooey innards of vis.c have any thoughts about this strategy?
I did some little research and noticed that the non-blocking clients were introduced by me in revision 491. After reading the commit message I roughly remember the reason for the change to non blocking: We were working on the 3D visualization tool s3d and could bring the vis server to a standstill by running the TCP client (meshs3d) in gdb and stopping its execution. The TCP client was not killed but the client would not read from the socket, hence the TCP connection was still open but the TCP write call would not come back either and hang forever. This solution was a quick fix which probably is far from being perfect.
While searching for some info on that topic I found an interesting page which might prove helpful: http://blog.netherlabs.nl/articles/2009/01/18/the- ultimate-so_linger-page-or-why-is-my-tcp-not-reliable
Regards, Marek
I looked at the site and it talked about something I had already tried which works well. Here is another patch hope its better.
Signed-off-by: Jonathan Mzengeza jtmze87@gmail.com Index: vis.c =================================================================== --- vis.c (revision 1343) +++ vis.c (working copy) @@ -566,6 +566,7 @@ buffer_t *last_send = NULL; size_t ret; char* send_buffer = NULL; + char tmp[4096];
while ( !is_aborted() ) {
@@ -600,6 +601,17 @@
}
+ shutdown(thread_data->socket, SHUT_WR); + + for(;;) { + ret=read(thread_data->socket, tmp, sizeof(tmp)); + if(ret < 0) { + break; + } + if(!ret) { + break; + } + } if ( debug_level > 0 ) debug_output( "TCP client has left: %s \n", thread_data->ip );
On Friday 17 July 2009 15:10:55 jonathan mzengeza wrote:
I looked at the site and it talked about something I had already tried which works well. Here is another patch hope its better.
- shutdown(thread_data->socket, SHUT_WR);
Does your patch also work without the shutdown ? Some clients (e.g. s3d) read the constant stream of vis data to update their visualization without re- opening the connection.
Regards, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Antoine van Gelder -
jonathan mzengeza -
Marek Lindner -
Sven Eckelmann