Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-1290

thrift: TNonblockingServer: clean up state in the

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • None
    • 0.8
    • C++ - Library
    • None

    Description

      From cd9c1a10cb4df058fbdbed1b98a21a7a7470a28c Mon Sep 17 00:00:00 2001
      From: Adam Simpkins <simpkins@fb.com>
      Date: Tue, 6 Apr 2010 20:43:23 +0000
      Subject: [PATCH 17/33] thrift: TNonblockingServer: clean up state in the
      destructor

      Summary:
      Implement the TNonblockingServer destructor, so that it closes the
      listen socket, destroys the event_base, and deletes any TConnections
      left in the connectionStack_. However, TNonblockingServer doesn't keep
      track of active TConnection objects, so those objects are still leaked.

      As part of this, I also changed the code to use event_init() rather than
      event_base_new(). This way we won't set the global event_base inside
      libevent, and we can be sure that no one else will be using it after the
      TNonblockingServer is destroyed.

      I grepped through all of [fb code base] to check for any other direct uses of
      event_set(), and didn't see any places that weren't also using
      event_base_set(). Therefore it seems like it should be safe to stop
      initializing the global event_base pointer.

      Test Plan:
      Tested with the test code in [a fb unittest], which creates, stops, and then
      deletes several TNonblockingServers. Ran it under valgrind, and now it
      only complains about any active connections being leaked.

      Revert Plan:
      OK

      —
      lib/cpp/src/server/TNonblockingServer.cpp | 4 ++--
      1 files changed, 2 insertions, 2 deletions

      Attachments

        Activity

          bryanduxbury Bryan Duxbury added a comment -

          Hm, this patch doesn't apply cleanly for me.

          bryanduxbury Bryan Duxbury added a comment - Hm, this patch doesn't apply cleanly for me.
          davejwatson@fb Dave Watson added a comment -

          svn up'd, resolved conflict

          davejwatson@fb Dave Watson added a comment - svn up'd, resolved conflict
          bryanduxbury Bryan Duxbury added a comment -

          I just committed this.

          bryanduxbury Bryan Duxbury added a comment - I just committed this.
          hudson Hudson added a comment -

          Integrated in Thrift #235 (See https://builds.apache.org/job/Thrift/235/)
          THRIFT-1290. cpp: TNonblockingServer: clean up state in the
          destructor

          Patch: Adam Simpkins

          bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1161655
          Files :

          • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp
          hudson Hudson added a comment - Integrated in Thrift #235 (See https://builds.apache.org/job/Thrift/235/ ) THRIFT-1290 . cpp: TNonblockingServer: clean up state in the destructor Patch: Adam Simpkins bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1161655 Files : /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp

          People

            davejwatson@fb Dave Watson
            davejwatson@fb Dave Watson
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: