You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2012/08/01 00:15:48 UTC

Review Request: Change IO buffer ownership strategy to avoid memory leaks

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

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

Re: Review Request: Change IO buffer ownership strategy to avoid memory leaks

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6257/#review10089
-----------------------------------------------------------

Ship it!


Ship It!

- Alan Conway


On Aug. 9, 2012, 8:55 p.m., Andrew Stitcher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6257/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2012, 8:55 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/broker/windows/SslProtocolFactory.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1370395 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIO.h 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.h 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/posix/AsynchIO.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.h 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.h 1370395 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.cpp 1370395 
> 
> 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
> 
>

Re: Review Request: Change IO buffer ownership strategy to avoid memory leaks

Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6257/
-----------------------------------------------------------

(Updated Aug. 9, 2012, 8:55 p.m.)


Review request for qpid, Alan Conway, Gordon Sim, and Steve Huston.


Changes
-------

Changes reworked a little to remove magic constants.

The HA tests now pass for me, but the original failure might well have been something screwed up in my test environment.

I would like to commit this change to trunk, so if there are concerns speak up now.


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 (updated)
-----

  /trunk/qpid/cpp/src/qpid/broker/windows/SslProtocolFactory.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1370395 
  /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/AsynchIO.h 1370395 
  /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.h 1370395 
  /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/posix/AsynchIO.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.h 1370395 
  /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1370395 
  /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1370395 
  /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.h 1370395 
  /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.cpp 1370395 

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

Re: Review Request: Change IO buffer ownership strategy to avoid memory leaks

Posted by Andrew Stitcher <as...@apache.org>.

> On Aug. 1, 2012, 6:40 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp, line 90
> > <https://reviews.apache.org/r/6257/diff/1/?file=131555#file131555line90>
> >
> >     Should this be using a max-frame-size from somewhere?

I agree it shouldn't be a magic number (I just copied the old magic number!).

This is on the listen side and the buffers have to be set up before doing any negotiation to find out the max frame size, so for simplicity we use the maximum possible frame size. On the connect side we do in fact use the max-frame-size option to size the buffers.

It would be possible though tricky to use some full sized buffers until we know the max-frame-size and then allocate all the buffers of this size for the connection.


> On Aug. 1, 2012, 6:40 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp, line 99
> > <https://reviews.apache.org/r/6257/diff/1/?file=131555#file131555line99>
> >
> >     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?

The "guarantee" is that this connection has not ever sent anything on this connection before and could only have received a single frame - this code path is only used when sending out the initial protocol header.

The new code has no mechanism for adding new buffers. This is probably an advantage as it does not allow memory growth due to outgoing message frames. If you can't get a buffer to write a frame you have to wait until one is available.

In practice the write side pretty much only uses one buffer currently as AsynchIO::writable() calls up into AsynchHandler::idle() which picks up a buffer encodes into it then queues it for writing and returns. Now AsynchIO::writable() writes this buffer and calls up again until nothing is queued after the upcall.

[The AsynchIO code allows for queuing of buffers to write, but it's never actually used in the current code paths. Actually I think in the current code paths you might only need a single buffer for read and a single buffer for write]


- Andrew


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


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

Re: Review Request: Change IO buffer ownership strategy to avoid memory leaks

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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
> 
>