Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.6
    • Fix Version/s: 0.7
    • Component/s: C++ Client
    • Labels:
      None
    • Environment:

      Linux dev-gmx001 2.6.31.8-GMX-CORE2_64 #1 SMP Tue Dec 15 10:21:34 CET 2009 x86_64 Intel(R) Xeon(R) CPU E5405 @ 2.00GHz GenuineIntel GNU/Linux
      Debian GNU/Linux 3.1
      gcc version 4.3.3 (Debian 4.3.3-13.1)

      Description

      Hi,
      with the following code we are able to reproduce a fd leak:

      #include <qpid/client/Connection.h>

      int main(int argc, char** argv)
      {
      while (1)

      { qpid::client::Connection connection; connection.open("localhost", 5672); // connection.close(); }

      }

      When closing the connection explicitely as shown in the comment the fd leak no longer occurs. It seems that the destructor of Connection lacks closing the connection.

      Regards,
      Daniel

        Activity

        Hide
        Steve Huston added a comment -

        The code is not set up to close on destruction; I added comments to that affect. If someone believes that the destructor should close, please reopen this and take it from there.

        Comment changed in trunk r908706.

        Show
        Steve Huston added a comment - The code is not set up to close on destruction; I added comments to that affect. If someone believes that the destructor should close, please reopen this and take it from there. Comment changed in trunk r908706.
        Hide
        Stefan Hausner added a comment -

        @Steve:

        Not closing the connection in the dtor conflicts with the RAII-Idiom: Every resource (a fd in this case) acquired must be freed upon destruction.

        Imo this bug should be reopened (normal users dont have the right to do this).

        Show
        Stefan Hausner added a comment - @Steve: Not closing the connection in the dtor conflicts with the RAII-Idiom: Every resource (a fd in this case) acquired must be freed upon destruction. Imo this bug should be reopened (normal users dont have the right to do this).
        Hide
        Steve Huston added a comment -

        Stefan, the constructor doesn't initialize the fd; the open() call does, so we're not really talking about RAII here. Thus, requiring a corresponding close() is legitimate.

        Also, if the destructor were to close the fd, we'd introduce subtle problems where objects may be copied and one of the copies goes out of scope, closing the fd out from under the others.

        Show
        Steve Huston added a comment - Stefan, the constructor doesn't initialize the fd; the open() call does, so we're not really talking about RAII here. Thus, requiring a corresponding close() is legitimate. Also, if the destructor were to close the fd, we'd introduce subtle problems where objects may be copied and one of the copies goes out of scope, closing the fd out from under the others.
        Hide
        Stefan Hausner added a comment - - edited

        @Steve:

        agreed, strictly speaking this is not RAII here

        But if copying of Connection objects is allowed (which I didnt consider it was) then you're absolutly right: closing the connection in dtor doesnt make any sense.

        Another aspects I'd like to add to this discussion:

        You're using a boost::shared_ptr<ConnectionImpl> to store the ConnectionImpl-Pointer. Why not associating the shared_ptr with the close() method on delete? This way you could copy the Connection object around and still be sure close() gets called on destruction of the last copy.

        Another reason for using this is Exception Safety. You can be sure your Connection get closed even without usage of try-catch.

        Show
        Stefan Hausner added a comment - - edited @Steve: agreed, strictly speaking this is not RAII here But if copying of Connection objects is allowed (which I didnt consider it was) then you're absolutly right: closing the connection in dtor doesnt make any sense. Another aspects I'd like to add to this discussion: You're using a boost::shared_ptr<ConnectionImpl> to store the ConnectionImpl-Pointer. Why not associating the shared_ptr with the close() method on delete? This way you could copy the Connection object around and still be sure close() gets called on destruction of the last copy. Another reason for using this is Exception Safety. You can be sure your Connection get closed even without usage of try-catch.
        Hide
        Alan Conway added a comment -

        Connections should automatically be cleaned up, looking at ConnectionImpl::~ConnectionImpl there is a clear attempt to do so. If it's leaking its a bug.

        Note that connection uses the PIMPL idio so client::Connection is really just a ref-counted pointer to a ConnectionImpl. So client::Connection can indeed be copied and client::~Connection should definitely not calll close()

        However once all the client::Connections are gone, ConnectionImpl::~ConnectionImpl should make sure the connection is closed.

        Show
        Alan Conway added a comment - Connections should automatically be cleaned up, looking at ConnectionImpl::~ConnectionImpl there is a clear attempt to do so. If it's leaking its a bug. Note that connection uses the PIMPL idio so client::Connection is really just a ref-counted pointer to a ConnectionImpl. So client::Connection can indeed be copied and client::~Connection should definitely not calll close() However once all the client::Connections are gone, ConnectionImpl::~ConnectionImpl should make sure the connection is closed.

          People

          • Assignee:
            Steve Huston
            Reporter:
            Daniel Etzold
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development