[B.A.T.M.A.N.] [vis] fixed partial json output

[B.A.T.M.A.N.] [vis] fixed partial json output

[jonathan mzengeza]

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 ) );

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[jonathan mzengeza]

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 ) );

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Antoine van Gelder]

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."

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

> > 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Antoine van Gelder]

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

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Marek Lindner]

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

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[jonathan mzengeza]

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 );

Re: [B.A.T.M.A.N.] [vis] fixed partial json output

[Marek Lindner]

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