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