Hi Willy,
On 2014/11/21 14:35, Willy Tarreau wrote:
Hi Godbach!
On Fri, Nov 21, 2014 at 11:02:52AM +0800, Godbach wrote:
Hi Willy,
On 2014/11/19 2:31, Willy Tarreau wrote:
On Tue, Nov 18, 2014 at 08:23:57PM +0200, Denys Fedoryshchenko wrote:
Thanks! Seems working for me :) Will test more tomorrow.
There's no reason it would not, otherwise we'd have a different bug.
When I'm unsure I ask for testing before committing, but here there
was no doubt once the issue was understood :-)
Willy
A so quick fix. Cool! :-)
In fact, I have also experienced this kind issue before. Of course it is
not caused by original HAProxy codes but my own codes added to HAProxy.
However, the real reason is the same as this issue: the memory allocated
from pool is not reset properly.
And that's intended. pool_alloc2() works exactly like malloc() : the caller
is responsible for initializing it if needed.
So I have an idea for this kind issue: how about HAProxy reset the
memory allocated from pool directly in pool_alloc2().
If we worry about that the performance may be decreased by calling
memset() in each pool_alloc2(), a new option which allows user to enable
or disable memset() in pool_alloc2() can be added into HAProxy.
We only do that (partially) when using memory poisonning/debugging (to
reproduce issues easier). Yes performance suffers a lot when doing so,
especially when using large buffers, and people using large buffers are
the ones who care the most about performance.
I'd agree to slightly change the pool_alloc2() to *always* memset the area
when memory poisonning is in place, so that developers can more easily
detect if they missed something. But I don't want to use memset all the
time as a brown paper bag so that developers don't have to be careful.
We're missing some doc of course, and people can get trapped from time
to time (and I do as well), so this is what we must improve, and not
have the code hide the bugs instead.
What is really needed is that each field of session/transaction is
documented : who uses it, when, and who's responsible for initializing
it. Here with the capture, I missed the fact that the captures are part
of a transaction, thus were initialized by the HTTP code, so when using
tcp without http, there's an issue... A simple comment like
/* initialized by http_init_txn() */
in front of the capture field in the struct would have been enough to
avoid this. This is what must be improved. We also need to write
developer guidelines to remind people to update the doc/comments when
modifying the API. I know it's not easy, I miss a number of them as
well.
Cheers,
Willy
Thank you. Got it.
I do agree with you. The developer should be clear that when and where
to initial the memory properly.
--
Best Regards,
Godbach
