Qpid
  1. Qpid
  2. QPID-3256

Application which uses Qpid (in my case Excel) hangs on shutdown

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8, 0.10
    • Fix Version/s: 0.12
    • Component/s: C++ Client
    • Labels:
      None
    • Environment:

      OS: Windows.
      Qpid is assembled as DLL.

      Description

      Hi All

      I encountered with strange behavior on shutdown when using qpid 0-8 and 0-10.

      When I use qpid in standalone console-application everything is ok. But when I use qpid in DLL which is loaded into Excel (as RTD module), Excel hangs on shutdown.

      I found out that in standalone application on shutdown I have next stack:

      qpidclientd.dll!qpid::client::`anonymous namespace'::IOThread::~IOThread() Line 138 C++
      qpidclientd.dll!`qpid::client::`anonymous namespace'::theIO'::`2'::`dynamic atexit destructor for 'io''() + 0xd bytes C++
      qpidclientd.dll!_CRT_INIT(void * hDllHandle=0x60080000, unsigned long dwReason=0, void * lpreserved=0x00000001) Line 449 C
      qpidclientd.dll!__DllMainCRTStartup(void * hDllHandle=0x60080000, unsigned long dwReason=0, void * lpreserved=0x00000001) Line 560 + 0x11 bytes C
      qpidclientd.dll!_DllMainCRTStartup(void * hDllHandle=0x60080000, unsigned long dwReason=0, void * lpreserved=0x00000001) Line 510 + 0x11 bytes C
      ntdll.dll!77b79960()
      [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]
      ntdll.dll!77b9a516()
      ntdll.dll!77b9a3b8()
      kernel32.dll!77657363()
      msvcr90d.dll!__crtExitProcess(int status=0) Line 732 C
      msvcr90d.dll!doexit(int code=0, int quick=0, int retcaller=0) Line 644 + 0x9 bytes C
      msvcr90d.dll!exit(int code=0) Line 412 + 0xd bytes C
      Test.exe!__tmainCRTStartup() Line 599 C
      Test.exe!mainCRTStartup() Line 403 C
      kernel32.dll!77653677()
      ntdll.dll!77b79f02()
      ntdll.dll!77b79ed5()

      And in this state all threads of application have been already terminated. The only thread is:

      1 > 21720 Main Thread Main Thread qpid::client::`anonymous namespace'::IOThread::~IOThread Normal 0

      so code from file ConnectionImpl.cpp works well:

      ~IOThread() {
      std::vector<Thread> threads;

      { ScopedLock<Mutex> l(threadLock); if (poller_) poller_->shutdown(); t.swap(threads); }
      for (std::vector<Thread>::iterator i = threads.begin(); i != threads.end(); ++i) { i->join(); }
      }


      BUT in Excel I get stack:

      qpidclientd.dll!qpid::client::`anonymous namespace'::IOThread::~IOThread() Line 130 C++
      qpidclientd.dll!`qpid::client::`anonymous namespace'::theIO'::`2'::`dynamic atexit destructor for 'io''() + 0xd bytes C++
      qpidclientd.dll!_CRT_INIT(void * hDllHandle=0x07700000, unsigned long dwReason=0, void * lpreserved=0x00000000) Line 449 C
      qpidclientd.dll!__DllMainCRTStartup(void * hDllHandle=0x07700000, unsigned long dwReason=0, void * lpreserved=0x00000000) Line 560 + 0x11 bytes C
      qpidclientd.dll!_DllMainCRTStartup(void * hDllHandle=0x07700000, unsigned long dwReason=0, void * lpreserved=0x00000000) Line 510 + 0x11 bytes C
      ntdll.dll!77b79960()
      [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]
      ntdll.dll!77ba1525()
      ntdll.dll!77b81231()
      KernelBase.dll!77281da7()
      ole32.dll!75bb9562()
      ole32.dll!75bb9593()
      ole32.dll!75bb95a7()
      ole32.dll!75bb98bf()
      ole32.dll!75bb9805()
      ole32.dll!75bb9a8c()
      EXCEL.EXE!2f3811e9()
      EXCEL.EXE!2f6933e8()
      EXCEL.EXE!2f32a5af()
      EXCEL.EXE!2f34894a()
      EXCEL.EXE!2f670001()
      MSO.DLL!65bc6ed5()
      MSO.DLL!65c26a34()
      MSO.DLL!65c30305()
      MSO.DLL!65bc910c()
      MSO.DLL!65c4f420()
      MSO.DLL!65bbf161()
      comctl32.dll!7233463d()
      user32.dll!762971be()
      user32.dll!76297d31()
      user32.dll!76297dfa()
      EXCEL.EXE!2f324572()
      EXCEL.EXE!2f324534()
      EXCEL.EXE!2f324441()
      MSO.DLL!65b78116()
      MSO.DLL!65ba1fd0()
      EXCEL.EXE!2f30424b()
      msvcr90.dll!749936c5()
      msvcr90.dll!749938b3()
      msvcr90.dll!749938c5()
      msvcr90.dll!749ac40c()
      msvcr90.dll!749b028d()
      msvcr90.dll!749b04f3()
      EXCEL.EXE!2f303f0a()
      kernel32.dll!77653677()
      ntdll.dll!77b79f02()
      ntdll.dll!77b79ed5()

      And threads:

      0 24016 Worker Thread _threadstartex _threadstartex Normal 0
      0 > 22928 Main Thread Main Thread qpid::client::`anonymous namespace'::IOThread::~IOThread Normal 0
      0 20224 RPC Thread RPC Callback Thread 77b5fd21 Normal 0
      0 16492 Worker Thread Win32 Thread 77b61ed6 Normal 0
      0 19948 Worker Thread Win32 Thread 77b600ed Normal 0
      0 20524 Worker Thread Win32 Thread 77b61ed6 Normal 0
      0 20532 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 21500 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 21848 Worker Thread Win32 Thread 77b61ed6 Normal 0
      0 22152 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 22164 Worker Thread Win32 Thread 77b5f8e9 Normal 0
      0 22300 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 22360 Worker Thread Win32 Thread 77b61ed6 Normal 0
      0 23316 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 23556 Worker Thread Win32 Thread 77b5f8e9 Normal 0
      0 23700 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 23912 Worker Thread Win32 Thread 77b5f8e9 Normal 0
      0 24276 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 24308 Worker Thread Win32 Thread 77b5f861 Normal 0
      0 24424 Worker Thread Win32 Thread 77b600ed Normal 0
      0 24452 Worker Thread Win32 Thread 77b5f8e9 Normal 0
      0 24520 Worker Thread Win32 Thread 77b61ed6 Normal 0

      As result this code (below) hangs application(Excel):

      ~IOThread() {
      std::vector<Thread> threads;
      { ScopedLock<Mutex> l(threadLock); if (poller_) poller_->shutdown(); t.swap(threads); }

      for (std::vector<Thread>::iterator i = threads.begin(); i != threads.end(); ++i)

      { i->join();-- APPLICATION HANGS HERE !!!! }

      }

      I suppose it occurs because of qpid tries to wait of thread completing from _DllMainCRTStartup which is called with PROCESS_DETACH.
      To work around it I modified function IOThread::sub():

      void sub() {
      std::vector<Thread> threads;
      {
      ScopedLock<Mutex> l(threadLock);
      --connections;

      if (connections == 0){
      if (poller_)

      { poller_->shutdown(); poller_.reset(); t.swap(threads); }

      }
      }
      for (std::vector<Thread>::iterator i = threads.begin(); i != threads.end(); ++i)

      { i->join(); }

      }

      But I don't think it is a good solution.
      Could you help me to solve this problem?
      Thanks

      1. qpid-3256.patch
        6 kB
        Cliff Jansen
      2. qpid-3256-3.patch
        10 kB
        Cliff Jansen

        Activity

        Hide
        Cliff Jansen added a comment -

        Applied to 0.12 branch r1149268

        Show
        Cliff Jansen added a comment - Applied to 0.12 branch r1149268
        Hide
        Cliff Jansen added a comment -

        Steve, thanks for looking into the nightly scoreboard build.

        Justin, I'm fine with this patch included now. I will apply it asap and send you mail.

        Show
        Cliff Jansen added a comment - Steve, thanks for looking into the nightly scoreboard build. Justin, I'm fine with this patch included now. I will apply it asap and send you mail.
        Hide
        Justin Ross added a comment -

        Cliff, I'd like to produce RC2 today, and if we want to go ahead with this patch, I'd like to get it in RC2 (RC3 would be too late). How does that strike you?

        Show
        Justin Ross added a comment - Cliff, I'd like to produce RC2 today, and if we want to go ahead with this patch, I'd like to get it in RC2 (RC3 would be too late). How does that strike you?
        Hide
        Steve Huston added a comment -

        The nightly scoreboard build on Windows was hanging up because of an svn conflict. That's resolved and now things appear to be good wrt this change.

        Show
        Steve Huston added a comment - The nightly scoreboard build on Windows was hanging up because of an svn conflict. That's resolved and now things appear to be good wrt this change.
        Hide
        Cliff Jansen added a comment -

        I wanted to see the patch run through Steve's nightly review board automated build and test.

        It started failing precisely at the same time I did the checkin to trunk, not a good sign. That plus the recent mingw build problem makes me wonder if it is a good idea to add it to the release at this time. The person who reported the JIRA has an earlier version of the patch that works for him.

        But I can't say for sure if there is a problem. It may be differences between my setup and Steve's, i.e. Xp versus WS2008, or different versions of boost or Visual Studio, or 32 vs 64 bit. Or the Riverace build may be failing for some unrelated reason.

        Show
        Cliff Jansen added a comment - I wanted to see the patch run through Steve's nightly review board automated build and test. It started failing precisely at the same time I did the checkin to trunk, not a good sign. That plus the recent mingw build problem makes me wonder if it is a good idea to add it to the release at this time. The person who reported the JIRA has an earlier version of the patch that works for him. But I can't say for sure if there is a problem. It may be differences between my setup and Steve's, i.e. Xp versus WS2008, or different versions of boost or Visual Studio, or 32 vs 64 bit. Or the Riverace build may be failing for some unrelated reason.
        Hide
        Steve Huston added a comment -

        I believe it's an important fix to get in.

        Show
        Steve Huston added a comment - I believe it's an important fix to get in.
        Hide
        Justin Ross added a comment -

        This landed after we branched for release, so I've adjusted the fix version to reflect that.

        I'd like to also consider introducing this for 0.12. Any thoughts or objections?

        Show
        Justin Ross added a comment - This landed after we branched for release, so I've adjusted the fix version to reflect that. I'd like to also consider introducing this for 0.12. Any thoughts or objections?
        Hide
        Cliff Jansen added a comment -

        I apologize for my lack of familiarity with mingw32.

        I believe that r1145876 will fix the syntax complaints and missing mingw32 definition.

        Show
        Cliff Jansen added a comment - I apologize for my lack of familiarity with mingw32. I believe that r1145876 will fix the syntax complaints and missing mingw32 definition.
        Hide
        Ted Ross added a comment -

        This commit appears to break the mingw32 build. There are two problems that I'm aware of:

        1) In the class ThreadPrivate, the initializers in the constructors are not in the same order as the declarations.
        2) error: 'OpenThread' was not declared in this scope from line 71.

        Show
        Ted Ross added a comment - This commit appears to break the mingw32 build. There are two problems that I'm aware of: 1) In the class ThreadPrivate, the initializers in the constructors are not in the same order as the declarations. 2) error: 'OpenThread' was not declared in this scope from line 71.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        Looks good to me, Cliff - thanks!

        • Steve

        On 2011-07-05 17:54:17, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-05 17:54:17)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review981 ----------------------------------------------------------- Ship it! Looks good to me, Cliff - thanks! Steve On 2011-07-05 17:54:17, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-05 17:54:17) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        Cliff,

        Thanks for your effort on this.

        Without this patch test cases would deadlock under NUnit probably the same as under Excel.
        With the patch the test cases exit and NUnit is happy.

        -Chuck

        • Chug

        On 2011-07-05 17:54:17, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-05 17:54:17)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review965 ----------------------------------------------------------- Ship it! Cliff, Thanks for your effort on this. Without this patch test cases would deadlock under NUnit probably the same as under Excel. With the patch the test cases exit and NUnit is happy. -Chuck Chug On 2011-07-05 17:54:17, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-05 17:54:17) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-07-05 17:54:17.154114)

        Review request for qpid.

        Changes
        -------

        Now uses Thread local storage (TLS) instead of a std::map to locate the ThreadPrivate structure. DllMain baggage still there to manage the FreeLibrary case.

        Summary
        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.
        https://issues.apache.org/jira/browse/qpid-3256

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151

        Diff: https://reviews.apache.org/r/987/diff

        Testing
        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-05 17:54:17.154114) Review request for qpid. Changes ------- Now uses Thread local storage (TLS) instead of a std::map to locate the ThreadPrivate structure. DllMain baggage still there to manage the FreeLibrary case. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs (updated) /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1143151 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        Cliff Jansen added a comment -

        New patch. Now uses Thread local storage (TLS) instead of a std::map to locate the ThreadPrivate structure. DllMain baggage still there to manage the FreeLibrary case.

        Show
        Cliff Jansen added a comment - New patch. Now uses Thread local storage (TLS) instead of a std::map to locate the ThreadPrivate structure. DllMain baggage still there to manage the FreeLibrary case.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-01 11:44:51, Steve Huston wrote:

        > My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work).

        Cliff Jansen wrote:

        Good point. I was so focused on the bug as presented I lost complete sight of this valid use case.

        I suppose I could do away with locks and maps altogether with use of TLS storage. In this case the only leaked resource would be the TLS slot on multiple uses of LoadLibrary/FreeLibrary, which is obviously not relevant in the static build case. So I should be able to use logic to free the slot in DllMain and comment out that code in a static build. Does this seem reasonable?

        Yes it does - thanks!

        • Steve

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

        On 2011-07-01 03:08:08, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-01 03:08:08)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-01 11:44:51, Steve Huston wrote: > My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work). Cliff Jansen wrote: Good point. I was so focused on the bug as presented I lost complete sight of this valid use case. I suppose I could do away with locks and maps altogether with use of TLS storage. In this case the only leaked resource would be the TLS slot on multiple uses of LoadLibrary/FreeLibrary, which is obviously not relevant in the static build case. So I should be able to use logic to free the slot in DllMain and comment out that code in a static build. Does this seem reasonable? Yes it does - thanks! Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review947 ----------------------------------------------------------- On 2011-07-01 03:08:08, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-01 03:08:08) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-01 11:44:51, Steve Huston wrote:

        > My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work).

        Good point. I was so focused on the bug as presented I lost complete sight of this valid use case.

        I suppose I could do away with locks and maps altogether with use of TLS storage. In this case the only leaked resource would be the TLS slot on multiple uses of LoadLibrary/FreeLibrary, which is obviously not relevant in the static build case. So I should be able to use logic to free the slot in DllMain and comment out that code in a static build. Does this seem reasonable?

        • Cliff

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

        On 2011-07-01 03:08:08, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-01 03:08:08)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-01 11:44:51, Steve Huston wrote: > My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work). Good point. I was so focused on the bug as presented I lost complete sight of this valid use case. I suppose I could do away with locks and maps altogether with use of TLS storage. In this case the only leaked resource would be the TLS slot on multiple uses of LoadLibrary/FreeLibrary, which is obviously not relevant in the static build case. So I should be able to use logic to free the slot in DllMain and comment out that code in a static build. Does this seem reasonable? Cliff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review947 ----------------------------------------------------------- On 2011-07-01 03:08:08, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-01 03:08:08) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        This was continued in a separate review request, https://reviews.apache.org/r/987/

        • Steve

        On 2011-06-15 01:08:18, Steve Huston wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/904/

        -----------------------------------------------------------

        (Updated 2011-06-15 01:08:18)

        Review request for qpid.

        Summary

        -------

        Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock.

        This addresses bug QPID-3256.

        https://issues.apache.org/jira/browse/QPID-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733

        Diff: https://reviews.apache.org/r/904/diff

        Testing

        -------

        Qpid regression test suite.

        Thanks,

        Steve

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/#review948 ----------------------------------------------------------- This was continued in a separate review request, https://reviews.apache.org/r/987/ Steve On 2011-06-15 01:08:18, Steve Huston wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/ ----------------------------------------------------------- (Updated 2011-06-15 01:08:18) Review request for qpid. Summary ------- Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock. This addresses bug QPID-3256 . https://issues.apache.org/jira/browse/QPID-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733 Diff: https://reviews.apache.org/r/904/diff Testing ------- Qpid regression test suite. Thanks, Steve
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Admin note... when this review is resolved, please ensure that https://reviews.apache.org/r/904/ is cleaned up accordingly.

        • Steve

        On 2011-07-01 03:08:08, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-01 03:08:08)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review949 ----------------------------------------------------------- Admin note... when this review is resolved, please ensure that https://reviews.apache.org/r/904/ is cleaned up accordingly. Steve On 2011-07-01 03:08:08, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-01 03:08:08) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work).

        • Steve

        On 2011-07-01 03:08:08, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/987/

        -----------------------------------------------------------

        (Updated 2011-07-01 03:08:08)

        Review request for qpid.

        Summary

        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.

        https://issues.apache.org/jira/browse/qpid-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687

        Diff: https://reviews.apache.org/r/987/diff

        Testing

        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/#review947 ----------------------------------------------------------- My concern with DllMain is that it precludes building qpid as static libraries (and having this logic still work). Steve On 2011-07-01 03:08:08, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- (Updated 2011-07-01 03:08:08) Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for qpid.

        Summary
        -------

        This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup.

        Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors.

        This addresses bug qpid-3256.
        https://issues.apache.org/jira/browse/qpid-3256

        Diffs


        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687

        Diff: https://reviews.apache.org/r/987/diff

        Testing
        -------

        Qpid cmake run_tests

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/987/ ----------------------------------------------------------- Review request for qpid. Summary ------- This is the same logic as the preceding version with naming fixes and refinements to DLL cleanup. Cleanup now uses Windows DllMain function to allows cleanup after C++ runtime static destructors. This addresses bug qpid-3256. https://issues.apache.org/jira/browse/qpid-3256 Diffs /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1141687 Diff: https://reviews.apache.org/r/987/diff Testing ------- Qpid cmake run_tests Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-16 12:56:46, Alan Conway wrote:

        > /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp, line 85

        > <https://reviews.apache.org/r/904/diff/1/?file=21139#file21139line85>

        >

        > Why the pointer? Just declare

        >

        > std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads;

        >

        > and let std::map take care of the memory management.

        Andrew Stitcher wrote:

        I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too)

        OK... this was previously invisible because I missed the <Publish> step

        The intended logic was that Thread::current() should have defined behavior, even if called from a static destructor. Otherwise, you need a way to guarantee the std:map destructor is never called too soon.

        The original JIRA's deadlock was in the context of a static destructor calling Thread::join().

        On re-examiniation, the use of a non-POD lock mechanism is inconsistent with the stated goal. Thank-you for the heads up. I will work with Steve to address this and the naming problem.

        I have a fixed version I will upload to a separate review... coming soon.

        • Cliff

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

        On 2011-06-15 01:08:18, Steve Huston wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/904/

        -----------------------------------------------------------

        (Updated 2011-06-15 01:08:18)

        Review request for qpid.

        Summary

        -------

        Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock.

        This addresses bug QPID-3256.

        https://issues.apache.org/jira/browse/QPID-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733

        Diff: https://reviews.apache.org/r/904/diff

        Testing

        -------

        Qpid regression test suite.

        Thanks,

        Steve

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-16 12:56:46, Alan Conway wrote: > /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp, line 85 > < https://reviews.apache.org/r/904/diff/1/?file=21139#file21139line85 > > > Why the pointer? Just declare > > std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads; > > and let std::map take care of the memory management. Andrew Stitcher wrote: I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too) OK... this was previously invisible because I missed the <Publish> step The intended logic was that Thread::current() should have defined behavior, even if called from a static destructor. Otherwise, you need a way to guarantee the std:map destructor is never called too soon. The original JIRA's deadlock was in the context of a static destructor calling Thread::join(). On re-examiniation, the use of a non-POD lock mechanism is inconsistent with the stated goal. Thank-you for the heads up. I will work with Steve to address this and the naming problem. I have a fixed version I will upload to a separate review... coming soon. Cliff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/#review849 ----------------------------------------------------------- On 2011-06-15 01:08:18, Steve Huston wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/ ----------------------------------------------------------- (Updated 2011-06-15 01:08:18) Review request for qpid. Summary ------- Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock. This addresses bug QPID-3256 . https://issues.apache.org/jira/browse/QPID-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733 Diff: https://reviews.apache.org/r/904/diff Testing ------- Qpid regression test suite. Thanks, Steve
        Hide
        Steve Huston added a comment -

        Reassigning this to Cliff at least until the review and new patch are resolved.

        Cliff, I still can't see your comments on the review board... can anyone else (Alan, Andrew)?

        Show
        Steve Huston added a comment - Reassigning this to Cliff at least until the review and new patch are resolved. Cliff, I still can't see your comments on the review board... can anyone else (Alan, Andrew)?
        Hide
        Cliff Jansen added a comment -

        The patch was published in the review board (https://reviews.apache.org/r/904/) where there was a comment regarding dynamic creation and deletion of the map of Qpid Thread objects:

        Alan Conway:

        Why the pointer? Just declare

        std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads;

        and let std::map take care of the memory management.

        Andrew Stitcher:

        I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too)

        Cliff Jansen:

        The intended logic was that Thread::current() should have defined behavior, even if called from a static destructor. Otherwise, you need a way to guarantee the std:map destructor is never called too soon.

        The original JIRA's deadlock was in the context of a static destructor calling Thread::join().

        On re-examiniation, the use of a non-POD lock mechanism is inconsistent with the stated goal. Thank-you for the heads up. I will work with Steve to address this and the naming problem.

        On yet further examination, even the PODMutex is not guaranteed to have defined behaviour during the static destructor resolution at process exit or library unload. The "static deinitialization order fiasco" has no current solution in the Boost mutex implementation (see http://www.justsoftwaresolutions.co.uk/articles/implementing_mutexes.html) despite attempts to address.

        I see three possible ways forward:

        1. Ship with the proposed fix (updated to address the review comments). It provides improved functionality, but a known potential random outage.

        2. Use a Windows CRITICAL_SECTION as the mutex, and initialize and release it outside the C++ runtime in DllMain. This is a common pattern in Windows and is in fact used in the DTC plugin module in the wcf subtree.

        3. Use a spin lock (via interlocked operations) to manage the exit/unload condition. Such a spin lock holds no system resources to leak and requires no explicit shutdown. This is the mechanism adopted by the Boost shared_ptr implementation of atomic operations.

        If anyone has a further suggestion, or a preference for option 1 please let me know. Otherwise I will put up examples of options 2 and 3 to the review board.

        Show
        Cliff Jansen added a comment - The patch was published in the review board ( https://reviews.apache.org/r/904/ ) where there was a comment regarding dynamic creation and deletion of the map of Qpid Thread objects: Alan Conway: Why the pointer? Just declare std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads; and let std::map take care of the memory management. Andrew Stitcher: I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too) Cliff Jansen: The intended logic was that Thread::current() should have defined behavior, even if called from a static destructor. Otherwise, you need a way to guarantee the std:map destructor is never called too soon. The original JIRA's deadlock was in the context of a static destructor calling Thread::join(). On re-examiniation, the use of a non-POD lock mechanism is inconsistent with the stated goal. Thank-you for the heads up. I will work with Steve to address this and the naming problem. On yet further examination, even the PODMutex is not guaranteed to have defined behaviour during the static destructor resolution at process exit or library unload. The "static deinitialization order fiasco" has no current solution in the Boost mutex implementation (see http://www.justsoftwaresolutions.co.uk/articles/implementing_mutexes.html ) despite attempts to address. I see three possible ways forward: 1. Ship with the proposed fix (updated to address the review comments). It provides improved functionality, but a known potential random outage. 2. Use a Windows CRITICAL_SECTION as the mutex, and initialize and release it outside the C++ runtime in DllMain. This is a common pattern in Windows and is in fact used in the DTC plugin module in the wcf subtree. 3. Use a spin lock (via interlocked operations) to manage the exit/unload condition. Such a spin lock holds no system resources to leak and requires no explicit shutdown. This is the mechanism adopted by the Boost shared_ptr implementation of atomic operations. If anyone has a further suggestion, or a preference for option 1 please let me know. Otherwise I will put up examples of options 2 and 3 to the review board.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-16 12:56:46, Alan Conway wrote:

        > /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp, line 85

        > <https://reviews.apache.org/r/904/diff/1/?file=21139#file21139line85>

        >

        > Why the pointer? Just declare

        >

        > std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads;

        >

        > and let std::map take care of the memory management.

        I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too)

        • Andrew

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

        On 2011-06-15 01:08:18, Steve Huston wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/904/

        -----------------------------------------------------------

        (Updated 2011-06-15 01:08:18)

        Review request for qpid.

        Summary

        -------

        Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock.

        This addresses bug QPID-3256.

        https://issues.apache.org/jira/browse/QPID-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733

        Diff: https://reviews.apache.org/r/904/diff

        Testing

        -------

        Qpid regression test suite.

        Thanks,

        Steve

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-16 12:56:46, Alan Conway wrote: > /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp, line 85 > < https://reviews.apache.org/r/904/diff/1/?file=21139#file21139line85 > > > Why the pointer? Just declare > > std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads; > > and let std::map take care of the memory management. I'd also add that we don't use the hungarian variable notation either so the 'p' should be stripped (possibly from other places too) Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/#review849 ----------------------------------------------------------- On 2011-06-15 01:08:18, Steve Huston wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/ ----------------------------------------------------------- (Updated 2011-06-15 01:08:18) Review request for qpid. Summary ------- Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock. This addresses bug QPID-3256 . https://issues.apache.org/jira/browse/QPID-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733 Diff: https://reviews.apache.org/r/904/diff Testing ------- Qpid regression test suite. Thanks, Steve
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp
        <https://reviews.apache.org/r/904/#comment1841>

        On linux this should be a PODMutex to ensure it is statically initialized before any possible use. Don't know if that's the case on Windows.

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp
        <https://reviews.apache.org/r/904/#comment1842>

        Why the pointer? Just declare

        std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads;

        and let std::map take care of the memory management.

        • Alan

        On 2011-06-15 01:08:18, Steve Huston wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/904/

        -----------------------------------------------------------

        (Updated 2011-06-15 01:08:18)

        Review request for qpid.

        Summary

        -------

        Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock.

        This addresses bug QPID-3256.

        https://issues.apache.org/jira/browse/QPID-3256

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733

        Diff: https://reviews.apache.org/r/904/diff

        Testing

        -------

        Qpid regression test suite.

        Thanks,

        Steve

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/#review849 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp < https://reviews.apache.org/r/904/#comment1841 > On linux this should be a PODMutex to ensure it is statically initialized before any possible use. Don't know if that's the case on Windows. /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp < https://reviews.apache.org/r/904/#comment1842 > Why the pointer? Just declare std::map<unsigned, ThreadPrivate::shared_ptr> pQpidThreads; and let std::map take care of the memory management. Alan On 2011-06-15 01:08:18, Steve Huston wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/ ----------------------------------------------------------- (Updated 2011-06-15 01:08:18) Review request for qpid. Summary ------- Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock. This addresses bug QPID-3256 . https://issues.apache.org/jira/browse/QPID-3256 Diffs ----- /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733 Diff: https://reviews.apache.org/r/904/diff Testing ------- Qpid regression test suite. Thanks, Steve
        Hide
        Cliff Jansen added a comment -

        I requested that the reporter test the patch in the JIRA and received the following response:

        from Smirnov Eugene eugenesm@yandex.ru
        to Cliff Jansen <cliffjansen@gmail.com>
        date Tue, Jun 14, 2011 at 10:41 AM
        subject Re: qpid 3256 excel hang

        Hi Cliff

        I've checked and it works fine.

        Thank you!!!

        Show
        Cliff Jansen added a comment - I requested that the reporter test the patch in the JIRA and received the following response: from Smirnov Eugene eugenesm@yandex.ru to Cliff Jansen <cliffjansen@gmail.com> date Tue, Jun 14, 2011 at 10:41 AM subject Re: qpid 3256 excel hang Hi Cliff I've checked and it works fine. Thank you!!!
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for qpid.

        Summary
        -------

        Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock.

        This addresses bug QPID-3256.
        https://issues.apache.org/jira/browse/QPID-3256

        Diffs


        /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733

        Diff: https://reviews.apache.org/r/904/diff

        Testing
        -------

        Qpid regression test suite.

        Thanks,

        Steve

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/904/ ----------------------------------------------------------- Review request for qpid. Summary ------- Keeps track of Qpid runnable threads and other threads, ensuring that rundown doesn't deadlock. This addresses bug QPID-3256 . https://issues.apache.org/jira/browse/QPID-3256 Diffs /trunk/qpid/cpp/src/qpid/sys/windows/Thread.cpp 1132733 Diff: https://reviews.apache.org/r/904/diff Testing ------- Qpid regression test suite. Thanks, Steve
        Hide
        Steve Huston added a comment -

        Taking over to apply attached patch for 0.12.

        Show
        Steve Huston added a comment - Taking over to apply attached patch for 0.12.
        Hide
        Steve Huston added a comment -

        All yours, Cliff... thanks.

        Show
        Steve Huston added a comment - All yours, Cliff... thanks.
        Hide
        Eugene added a comment -

        Cliff, Thank You for the answer.

        Show
        Eugene added a comment - Cliff, Thank You for the answer.
        Hide
        Cliff Jansen added a comment -

        I can reproduce the problem with a program that is not linked against
        qpidclientd but loads and unloads it directly. A deadlock occurs
        processing the global static destructors.

        Thread 1 blocks waiting for thread 2 to exit/die. It also owns the
        infamous loader lock (thanks to the use of FreeLibrary())

        ntdll!NtWaitForSingleObject+0xa
        KERNELBASE!WaitForSingleObjectEx+0x79
        qpidcommond!qpid::sys::Thread::join+0x66 [thread.cpp @ 82]
        qpidclientd!qpid::client::`anonymous namespace'::IOThread::~IOThread+0x141
        qpidclientd!`qpid::client::`anonymous namespace'::theIO'::`2'::`dynamic atexit destructor for 'io''+0x26
        qpidclientd!_CRT_INIT+0x2dc
        qpidclientd!__DllMainCRTStartup+0x11f
        qpidclientd!_DllMainCRTStartup+0x31
        ntdll!LdrpUnloadDll+0x27d
        ntdll!LdrUnloadDll+0x4a
        KERNELBASE!FreeLibrary+0x1d
        foo!main+0x112

        Thread 2 is a zombie trying to die, if only it could get the loader lock

        ntdll!NtWaitForSingleObject+0xa
        ntdll!RtlpWaitOnCriticalSection+0xe8
        ntdll!RtlEnterCriticalSection+0xd1
        ntdll!LdrShutdownThread+0x72
        ntdll!RtlExitUserThread+0x38
        MSVCR90D!_endthreadex+0x33
        qpidcommond!`anonymous namespace'::runRunnable+0x3b [thread.cpp @ 34]
        MSVCR90D!_callthreadstartex+0x25
        MSVCR90D!_threadstartex+0xbd
        kernel32!BaseThreadInitThunk+0xd
        ntdll!RtlUserThreadStart+0x1d

        Deadlock: Thread 1 does not wakeup until Thread 2 really dies, hence
        neither thread can make progress.

        Thread 2 has nothing left to do except the Windows DLL_THREAD_DETACH
        handshake in all associated DLLs. It has clearly finished from the
        point of view of the Windows implementation of the Qpid Thread class.

        One way to fix this would be to use a private synchronization
        mechanism that completes before the call to endthreadex(). I will put
        a patch together for review along these lines unless someone objects.

        Show
        Cliff Jansen added a comment - I can reproduce the problem with a program that is not linked against qpidclientd but loads and unloads it directly. A deadlock occurs processing the global static destructors. Thread 1 blocks waiting for thread 2 to exit/die. It also owns the infamous loader lock (thanks to the use of FreeLibrary()) ntdll!NtWaitForSingleObject+0xa KERNELBASE!WaitForSingleObjectEx+0x79 qpidcommond!qpid::sys::Thread::join+0x66 [thread.cpp @ 82] qpidclientd!qpid::client::`anonymous namespace'::IOThread::~IOThread+0x141 qpidclientd!`qpid::client::`anonymous namespace'::theIO'::`2'::`dynamic atexit destructor for 'io''+0x26 qpidclientd!_CRT_INIT+0x2dc qpidclientd!__DllMainCRTStartup+0x11f qpidclientd!_DllMainCRTStartup+0x31 ntdll!LdrpUnloadDll+0x27d ntdll!LdrUnloadDll+0x4a KERNELBASE!FreeLibrary+0x1d foo!main+0x112 Thread 2 is a zombie trying to die, if only it could get the loader lock ntdll!NtWaitForSingleObject+0xa ntdll!RtlpWaitOnCriticalSection+0xe8 ntdll!RtlEnterCriticalSection+0xd1 ntdll!LdrShutdownThread+0x72 ntdll!RtlExitUserThread+0x38 MSVCR90D!_endthreadex+0x33 qpidcommond!`anonymous namespace'::runRunnable+0x3b [thread.cpp @ 34] MSVCR90D!_callthreadstartex+0x25 MSVCR90D!_threadstartex+0xbd kernel32!BaseThreadInitThunk+0xd ntdll!RtlUserThreadStart+0x1d Deadlock: Thread 1 does not wakeup until Thread 2 really dies, hence neither thread can make progress. Thread 2 has nothing left to do except the Windows DLL_THREAD_DETACH handshake in all associated DLLs. It has clearly finished from the point of view of the Windows implementation of the Qpid Thread class. One way to fix this would be to use a private synchronization mechanism that completes before the call to endthreadex(). I will put a patch together for review along these lines unless someone objects.
        Hide
        Eugene added a comment -

        I check issue with qpid 0.10 and get the same problem.

        Show
        Eugene added a comment - I check issue with qpid 0.10 and get the same problem.
        Hide
        Eugene added a comment -

        Steve, I have reviewed src\qpid\client\ConnectionImpl.cpp for last version and don't find any changes(may be I'm wrong).
        I think the main problem is in using static object:
        IOThread& theIO()

        { static IOThread io(SystemInfo::concurrency()); return io; }

        which is destroyed on shutdown and tries wait for thread completing in destructor.

        Show
        Eugene added a comment - Steve, I have reviewed src\qpid\client\ConnectionImpl.cpp for last version and don't find any changes(may be I'm wrong). I think the main problem is in using static object: IOThread& theIO() { static IOThread io(SystemInfo::concurrency()); return io; } which is destroyed on shutdown and tries wait for thread completing in destructor.
        Hide
        Steve Huston added a comment -

        Qpid 0.10 was released recently. Could you please obtain 0.10 and retry your case with that?

        Show
        Steve Huston added a comment - Qpid 0.10 was released recently. Could you please obtain 0.10 and retry your case with that?

          People

          • Assignee:
            Cliff Jansen
            Reporter:
            Eugene
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development