[noPoll] Stress Test -- Final Flag after calling nopoll_conn_get_msg
Kale, Rahul
Rahul.Kale en barco.com
Mar Jun 14 18:50:43 CEST 2016
Hello,
Any update on this issue yet?
On second thoughts, I believe that noPoll library should also attempt to
consolidate multiple fragments into a complete message when
nopoll_conn_get_msg() is used.
A typical use case for websockets is to send JSON or XML text messages back
and forth. Forcing the user to worry about coding up stitching together
potentially fragmented messages is an unnecessary burden. Websockets
specifically provides a way to have a message based API and indicate end of
message using final bit.
The noPoll library already provides a stream based API. The user should be
able to pick the appropriate API (message based vs. stream based) depending
on the use case. Presumably the user of the library is fully aware about the
nature of messages transferred and the expected size of the messages. To
prevent blow up of the memory, the library should expose configurable
parameters for maximum size of each message and total size of fragmented
messages. Once these are exceeded, it should be OK for the noPoll library to
abort the connection. The user can configure these parameters based on domain
knowledge and tolerance for memory consumption.
I noticed that other websocket libraries already follow this approach.
For example:
https://github.com/theturtle32/WebSocket-Node/blob/master/docs/WebSocketClient.md
Best Regards,
Rahul
> -----Original Message-----
> From: Kale, Rahul
> Sent: Friday, June 03, 2016 1:22 PM
> To: nopoll en lists.aspl.es
> Cc: francis.brosnan en aspl.es
> Subject: RE: Stress Test -- Final Flag after calling nopoll_conn_get_msg
>
>
>
>
> Hello,
>
> I believe I have a resolution for the incorrect final flag reported on the
> message returned by nopoll_conn_get_msg();
>
> First off, I determined that this is a receiver side issue since the problem was
> reproducible even with the Node.js version of the server side.
>
> From the documentation for noPoll:
>
> "In general noPoll tries to consolidate internal frame fragments but only for
> some particular functions like (nopoll_conn_get_msg). For example, ..."
>
> Looking at the code for nopoll_conn_get_msg() I believe that this is not what
> is being done. When a partial frame (header and part of the body) is
> received, it is handed back directly to the caller. It is made to look like a
> fragmented message with adjustments made for is_fragment() and
> is_final().
>
> Indeed your approach is reasonable. We should NOT attempt to
> automatically integrate fragments into a single message. Otherwise there is a
> danger of running out of memory. On the wire, a sender is free to send an
> arbitrarily long message. Potentially, an infinitely long stream of fragmented
> messages can be send with a 'final' flag set after even gigabytes. I believe
> currently there is no upper limit set on the size of a complete message in the
> Websocket specification.
>
> The adjustments made for is_final() flag for partial frame received did not
> seem to be correct. I have attempted to fix this by doing the following
> changes. First, I introduced a new 'is_fin' flag in the noPollMsg structure.
> I ensure that the original 'has_fin' flag is only initialized from the first header
> received. That flag is transfered to the next message structure created in
> case the previous one was partial frame. The new 'is_fin' flag is set to false
> when delivering a partial frame. It is set to the original 'has_fin' value when
> the final portion of the frame is delivered. Finally, the nopoll_msg_is_final()
> method now returns the new 'is_fin' flag instead of the 'has_fin' flag.
>
> The reasoning is that the original incoming message frame might also be itself
> fragmented and the value for the final flag in the last segment delivered
> needs to reflect that.
>
> After the above changes, the reported is_final() flag is always correct for all
> messages received even under stress.
>
> I do not have a full understanding of the code so you may have a better
> approach to fix this. Anyway, below is a unified diff of the changes I made:
>
>
> diff -u -r -w ../tmp/nopoll-0.4.1.b265/src/nopoll_conn.c ./src/nopoll_conn.c
> --- ../tmp/nopoll-0.4.1.b265/src/nopoll_conn.c 2016-05-23
> 12:01:54.000000000 -0700
> +++ ./src/nopoll_conn.c 2016-06-03 12:06:28.502322481 -0700
> @@ -2752,7 +2752,8 @@
> msg->is_fragment = nopoll_true;
>
> /* get fin bytes */
> - msg->has_fin = 1; /* for now final message */
> + msg->has_fin = conn->previous_msg->has_fin;
> + msg->is_fin = nopoll_false;
> msg->op_code = 0; /* continuation frame */
>
> /* copy initial mask indication */
> @@ -3069,11 +3070,16 @@
>
> /* flag this message as a fragment */
> msg->is_fragment = nopoll_true;
> + msg->is_fin = nopoll_false;
>
> /* flag that this message doesn't have FIN = 0 because
> * we wasn't able to read it entirely */
> /* msg->has_fin = 0; */
> +
> } /* end if */
> + else {
> + msg->is_fin = msg->has_fin;
> + }
>
> /* flag the message was being a fragment according to previous flag
> */
> msg->is_fragment = msg->is_fragment || conn-
> >previous_was_fragment || msg->has_fin == 0;
>
> diff -u -r -w ../tmp/nopoll-0.4.1.b265/src/nopoll_msg.c ./src/nopoll_msg.c
> --- ../tmp/nopoll-0.4.1.b265/src/nopoll_msg.c 2015-11-15
> 01:57:52.000000000 -0800
> +++ ./src/nopoll_msg.c 2016-06-03 11:52:28.102344864 -0700
> @@ -180,7 +180,7 @@
> if (msg == NULL)
> return nopoll_false;
>
> - return msg->has_fin;
> + return msg->is_fin;
> }
>
> /**
>
> diff -u -r -w ../tmp/nopoll-0.4.1.b265/src/nopoll_private.h
> ./src/nopoll_private.h
> --- ../tmp/nopoll-0.4.1.b265/src/nopoll_private.h 2016-04-21
> 01:18:52.000000000 -0700
> +++ ./src/nopoll_private.h 2016-06-03 11:25:52.000716385 -0700
> @@ -358,6 +358,7 @@
> int remain_bytes;
>
> nopoll_bool is_fragment;
> + nopoll_bool is_fin;
> int unmask_desp;
> };
>
>
>
> Regards,
>
> Rahul
>
>
>
> Rahul Kale
>
> IP Video Systems
> Barco, Inc
> 1287 Anvilwood Ave
> Sunnyvale, CA 94089
>
> Tel +1 408 400 4238
This message is subject to the following terms and conditions: MAIL DISCLAIMER<http://www.barco.com/en/maildisclaimer>
Más información sobre la lista de distribución noPoll