qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request: Change IO buffer ownership strategy to avoid memory leaks
Date Wed, 01 Aug 2012 18:40:14 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6257/#review9702
-----------------------------------------------------------



/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/6257/#comment20671>

    Should this be using a max-frame-size from somewhere?



/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/6257/#comment20672>

    What guarantees this assertion will pass? Looks like AsynchIO::getQueuedBuffer() can return
0. How does the new code grow the number of buffers if required?


- Alan Conway


On July 31, 2012, 10:15 p.m., Andrew Stitcher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6257/
> -----------------------------------------------------------
> 
> (Updated July 31, 2012, 10:15 p.m.)
> 
> 
> Review request for qpid, Alan Conway, Gordon Sim, and Steve Huston.
> 
> 
> Description
> -------
> 
> This change reworks the buffer handling of the IO layer of qpid.
> 
> Essentially it makes the AsyncIO class the owner of the IO buffers always, previously
it only owned the buffers when they were on one of its queues. This allowed the buffers to
leak if they were not returned to its queues for some reason (perhaps an exception being thrown).
> 
> In order to apply this change to the SSL code as well I've had to bring SslConnector
in line with the current TCPConnector code which it has markedly diverged from.
> 
> I'm particularly seeking review on the Windows code which has been lightly tested, and
the SSL code because the change is large there (although it brings the SSL code much more
in line with the TCP code).
> 
> 
> This addresses bug QPID-4180.
>     https://issues.apache.org/jira/browse/QPID-4180
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1367696 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIO.h 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/posix/AsynchIO.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.h 1367696 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.cpp 1367696 
> 
> Diff: https://reviews.apache.org/r/6257/diff/
> 
> 
> Testing
> -------
> 
> Builds on Fedora 16, and Windows
> 
> make check
> cmake test
> 
> Both pass Except ha_tests which fail in the same way on the equivalent trunk version
for me.
> 
> Linux performace using qpid-cpp-benchmark seems comparable.
> 
> 
> Thanks,
> 
> Andrew Stitcher
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message