Re: [libmicrohttpd] Some issues with MHD

[Olivier Delhomme]

Hi Christian,

On Fri, Nov 06, 2015 at 11:00:05PM +0100, Christian Grothoff wrote:
> Reproduced, located, fixed: SVN 36643 (diff attached).
> Basically, contrary to what I was thinking, MHD wasn't somehow having an
> issue with the pool having allocations in it that would limit the
> available memory. The issue was simply that MHD would at the end of
> reading a request shrink the read buffer to the size that was needed for
> the request (to have more space for the write buffer). Then, for the
> next request, it would *only* grow the read buffer if it was strictly
> necessary (and not use the usual more generous allocation of half of the
> memory anymore).  That's also why it never failed: if the read buffer
> was actually to small, MHD would eventually grow it. But that would only
> happen after the IO buffer had gotten MUCH too small for performance.
> 
> Combined with the fact that it would take a while for the shrinkage to
> kick in (long-lasting multi-request POST session), this was not commonly
> observed.
> 
> Anyway, should now really be killed. Thanks for the report & the testcase!

Yes :-)

Thanks a lot: tested against SVN 36643 I can not reproduce this 
now. Perfect.

Happy Hacking ;-) !

 
> On 11/06/2015 10:09 PM, Olivier Delhomme wrote:
> > On Wed, Nov 04, 2015 at 11:00:42PM +0100, Christian Grothoff wrote:
> >> Hmm. That's not even clearly indicative of an MHD-related issue.
> > 
> > I'm even not sure that I'm doing things the right way... 
> > 
> >> What you should look at is the buffer size MHD offers when calling
> >> recv() on the TCP stream.  The size of the buffer MHD offers to the
> >> application can be smaller simply because the OS didn't receive 13k data
> >> chunks between calls anymore -- be it because the sender is slower, TCP
> >> had a congestion event or your receiver happens to be draining the
> >> buffer slightly faster. So there are many _other_ reasons that could
> >> explain a decrease in upload_buffer_size.
> >>
> >> So to diagnose this better, you first should check if the cause is
> >> really the size of the IO buffer offered by MHD to the kernel. Given
> >> that this is all for the same connection, I doubt this is the cause ---
> >> unless your application again doesn't always handle 100% of the data in
> >> each call, then you could of course also see such patterns emerge.
> > 
> > I'm sorry but I'm not sure to understand what your are saying as I'm
> > not an expert in TCP nor network related kernel things.
> > 
> > To diagnose it I managed to code a minimalistic server (160 lines with
> > comments) and a minimalistic client (200 lines with comments) that 
> > simulates the behavior of my programs. It is published here:
> > https://github.com/dupgit/shrinkage
> > 
> > I found the same behavior regarding the *upload_size value as you can
> > see in the *.png images and the file shrinkserver.output. The whole
> > run to observe this is about 6-7 minutes. I use the lo interface and
> > the throughput is around 80 Mbits/s. It has been tested on my laptop
> > that was running gnome desktop with very few opened applications.
> > The distribution is a Debian Jessie. shrinkserver is linked with
> > libmicrohttpd r36641 and shrinkclient is linked with system's libcurl
> > 7.38.0-4+deb8u2.
> > 
> > Can you tell me if this code leads to something similar with your
> > environment ?

> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 36640)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,6 @@
> +Fri Nov  6 22:54:38 CET 2015
> +     Fixing the buffer shrinkage issue, this time with test. -CG
> +
>  Tue Nov  3 23:24:52 CET 2015
>       Undoing change from Sun Oct 25 15:29:23 CET 2015
>       as the original code was counter-intuitive but
> Index: src/include/microhttpd.h
> ===================================================================
> --- src/include/microhttpd.h  (revision 36640)
> +++ src/include/microhttpd.h  (working copy)
> @@ -130,7 +130,7 @@
>   * Current version of the library.
>   * 0x01093001 = 1.9.30-1.
>   */
> -#define MHD_VERSION 0x00094600
> +#define MHD_VERSION 0x00094601
>  
>  /**
>   * MHD-internal return code for "YES".
> Index: src/microhttpd/connection.c
> ===================================================================
> --- src/microhttpd/connection.c       (revision 36640)
> +++ src/microhttpd/connection.c       (working copy)
> @@ -2559,14 +2559,15 @@
>                /* can try to keep-alive */
>                connection->version = NULL;
>                connection->state = MHD_CONNECTION_INIT;
> -              /* read_buffer_size is correct here, as we want to
> -                 preserve the entire read buffer allocation, even
> -                 if in terms of the data we only care to preserve
> -                 up to "read_buffer_offset" */
> +              /* Reset the read buffer to the starting size,
> +                 preserving the bytes we have already read. */
>                connection->read_buffer
>                  = MHD_pool_reset (connection->pool,
>                                    connection->read_buffer,
> -                                  connection->read_buffer_size);
> +                                  connection->read_buffer_offset,
> +                                  connection->daemon->pool_size / 2);
> +              connection->read_buffer_size
> +                = connection->daemon->pool_size / 2;
>              }
>         connection->client_aware = MHD_NO;
>            connection->client_context = NULL;
> Index: src/microhttpd/memorypool.c
> ===================================================================
> --- src/microhttpd/memorypool.c       (revision 36640)
> +++ src/microhttpd/memorypool.c       (working copy)
> @@ -219,7 +219,8 @@
>    if ((pool->end < old_size) || (pool->end < asize))
>      return NULL;                /* unsatisfiable or bogus request */
>  
> -  if ((pool->pos >= old_size) && (&pool->memory[pool->pos - old_size] == 
> old))
> +  if ( (pool->pos >= old_size) &&
> +       (&pool->memory[pool->pos - old_size] == old) )
>      {
>        /* was the previous allocation - optimize! */
>        if (pool->pos + asize - old_size <= pool->end)
> @@ -251,32 +252,40 @@
>  
>  /**
>   * Clear all entries from the memory pool except
> - * for @a keep of the given @a size.
> + * for @a keep of the given @a size. The pointer
> + * returned should be a buffer of @a new_size where
> + * the first @a copy_bytes are from @a keep.
>   *
>   * @param pool memory pool to use for the operation
>   * @param keep pointer to the entry to keep (maybe NULL)
> - * @param size how many bytes need to be kept at this address
> + * @param copy_bytes how many bytes need to be kept at this address
> + * @param new_size how many bytes should the allocation we return have?
> + *                 (should be larger or equal to @a copy_bytes)
>   * @return addr new address of @a keep (if it had to change)
>   */
>  void *
>  MHD_pool_reset (struct MemoryPool *pool,
>               void *keep,
> -             size_t size)
> +             size_t copy_bytes,
> +                size_t new_size)
>  {
>    if (NULL != keep)
>      {
>        if (keep != pool->memory)
>          {
> -          memmove (pool->memory, keep, size);
> +          memmove (pool->memory,
> +                   keep,
> +                   copy_bytes);
>            keep = pool->memory;
>          }
>      }
>    pool->end = pool->size;
> -  memset (&pool->memory[size],
> +  /* technically not needed, but safer to zero out */
> +  memset (&pool->memory[copy_bytes],
>         0,
> -       pool->size - size);
> +       pool->size - copy_bytes);
>    if (NULL != keep)
> -    pool->pos = ROUND_TO_ALIGN(size);
> +    pool->pos = ROUND_TO_ALIGN (new_size);
>    return keep;
>  }
>  
> Index: src/microhttpd/memorypool.h
> ===================================================================
> --- src/microhttpd/memorypool.h       (revision 36640)
> +++ src/microhttpd/memorypool.h       (working copy)
> @@ -99,16 +99,21 @@
>  
>  /**
>   * Clear all entries from the memory pool except
> - * for "keep" of the given "size".
> + * for @a keep of the given @a copy_bytes.  The pointer
> + * returned should be a buffer of @a new_size where
> + * the first @a copy_bytes are from @a keep.
>   *
>   * @param pool memory pool to use for the operation
>   * @param keep pointer to the entry to keep (maybe NULL)
> - * @param size how many bytes need to be kept at this address
> - * @return addr new address of "keep" (if it had to change)
> + * @param copy_bytes how many bytes need to be kept at this address
> + * @param new_size how many bytes should the allocation we return have?
> + *                 (should be larger or equal to @a copy_bytes)
> + * @return addr new address of @a keep (if it had to change)
>   */
>  void *
>  MHD_pool_reset (struct MemoryPool *pool,
>               void *keep,
> -             size_t size);
> +             size_t copy_bytes,
> +                size_t new_size);
>  
>  #endif


-- 
"Quand la vérité n'est pas libre, la liberté n'est pas vraie."
                                             [Jacques Prévert]