From 2a90e7de92bb7501fe369c31a8489c6b36cc8980 Mon Sep 17 00:00:00 2001 From: martin-osborne Date: Sun, 19 Oct 2014 10:59:08 +0100 Subject: [PATCH 1/2] Moved work for isue 532 into it's own branch. --- Foundation/include/Poco/Mutex_WIN32.h | 22 +++- Foundation/src/Mutex_WIN32.cpp | 94 ++++++++++++++ Foundation/testsuite/Makefile-Driver | 2 +- Foundation/testsuite/TestSuite.vxbuild | 1 + Foundation/testsuite/TestSuite_CE_vs90.vcproj | 8 ++ .../testsuite/TestSuite_WEC2013_vs110.vcxproj | 2 + .../TestSuite_WEC2013_vs110.vcxproj.filters | 6 + .../testsuite/TestSuite_WEC2013_vs120.vcxproj | 2 + .../TestSuite_WEC2013_vs120.vcxproj.filters | 6 + Foundation/testsuite/TestSuite_vs100.vcxproj | 2 + .../testsuite/TestSuite_vs100.vcxproj.filters | 6 + Foundation/testsuite/TestSuite_vs110.vcxproj | 2 + .../testsuite/TestSuite_vs110.vcxproj.filters | 6 + Foundation/testsuite/TestSuite_vs120.vcxproj | 2 + .../testsuite/TestSuite_vs120.vcxproj.filters | 6 + Foundation/testsuite/TestSuite_vs71.vcproj | 6 + Foundation/testsuite/TestSuite_vs80.vcproj | 8 ++ Foundation/testsuite/TestSuite_vs90.vcproj | 8 ++ .../testsuite/TestSuite_x64_vs100.vcxproj | 2 + .../TestSuite_x64_vs100.vcxproj.filters | 6 + .../testsuite/TestSuite_x64_vs110.vcxproj | 2 + .../TestSuite_x64_vs110.vcxproj.filters | 6 + .../testsuite/TestSuite_x64_vs120.vcxproj | 2 + .../TestSuite_x64_vs120.vcxproj.filters | 6 + .../testsuite/TestSuite_x64_vs90.vcproj | 8 ++ Foundation/testsuite/src/MutexTest.cpp | 122 ++++++++++++++++++ Foundation/testsuite/src/MutexTest.h | 40 ++++++ .../testsuite/src/ThreadingTestSuite.cpp | 2 + 28 files changed, 383 insertions(+), 2 deletions(-) create mode 100644 Foundation/testsuite/src/MutexTest.cpp create mode 100644 Foundation/testsuite/src/MutexTest.h diff --git a/Foundation/include/Poco/Mutex_WIN32.h b/Foundation/include/Poco/Mutex_WIN32.h index 110fcf4b7..dc871bfb3 100644 --- a/Foundation/include/Poco/Mutex_WIN32.h +++ b/Foundation/include/Poco/Mutex_WIN32.h @@ -43,7 +43,20 @@ private: }; -typedef MutexImpl FastMutexImpl; +class Foundation_API FastMutexImpl +{ +protected: + FastMutexImpl(); + ~FastMutexImpl(); + void lockImpl(); + bool tryLockImpl(); + bool tryLockImpl(long milliseconds); + void unlockImpl(); + +private: + CRITICAL_SECTION _cs; + int _lockCount; +}; // @@ -81,6 +94,13 @@ inline void MutexImpl::unlockImpl() } +inline void FastMutexImpl::unlockImpl() +{ + --_lockCount; + LeaveCriticalSection(&_cs); +} + + } // namespace Poco diff --git a/Foundation/src/Mutex_WIN32.cpp b/Foundation/src/Mutex_WIN32.cpp index e80064f77..5700cc35b 100644 --- a/Foundation/src/Mutex_WIN32.cpp +++ b/Foundation/src/Mutex_WIN32.cpp @@ -15,6 +15,7 @@ #include "Poco/Mutex_WIN32.h" +#include "Poco/Thread.h" #include "Poco/Timestamp.h" @@ -58,4 +59,97 @@ bool MutexImpl::tryLockImpl(long milliseconds) } +FastMutexImpl::FastMutexImpl() + : _lockCount(0) +{ + // the fct has a boolean return value under WInnNt/2000/XP but not on Win98 + // the return only checks if the input address of &_cs was valid, so it is safe to omit it + InitializeCriticalSectionAndSpinCount(&_cs, 4000); +} + + +FastMutexImpl::~FastMutexImpl() +{ + DeleteCriticalSection(&_cs); +} + + +void FastMutexImpl::lockImpl() +{ + try + { + EnterCriticalSection(&_cs); + ++_lockCount; + + if (_lockCount > 1) + { + // We're trying to go recursive so self-deadlock + Thread::current()->join(); + } + } + catch (...) + { + throw SystemException("cannot lock mutex"); + } +} + + +bool FastMutexImpl::tryLockImpl() +{ + try + { + if (TryEnterCriticalSection(&_cs) == 0) + return false; + + if (_lockCount > 0) + { + // We're trying to go recursive + LeaveCriticalSection(&_cs); + return false; + } + + ++_lockCount; + return true; + } + catch (...) + { + } + throw SystemException("cannot lock mutex"); +} + + +bool FastMutexImpl::tryLockImpl(long milliseconds) +{ + const int sleepMillis = 5; + Timestamp now; + Timestamp::TimeDiff diff(Timestamp::TimeDiff(milliseconds)*1000); + + do + { + try + { + if (TryEnterCriticalSection(&_cs) == TRUE) + { + if (_lockCount == 0) + { + ++_lockCount; + return true; + } + + // We're trying to go recursive + LeaveCriticalSection(&_cs); + Sleep(milliseconds); + return false; + } + } + catch (...) + { + throw SystemException("cannot lock mutex"); + } + Sleep(sleepMillis); + } while (!now.isElapsed(diff)); + return false; +} + + } // namespace Poco diff --git a/Foundation/testsuite/Makefile-Driver b/Foundation/testsuite/Makefile-Driver index f6e6ed18b..e8c63c2f2 100644 --- a/Foundation/testsuite/Makefile-Driver +++ b/Foundation/testsuite/Makefile-Driver @@ -24,7 +24,7 @@ objects = ActiveMethodTest ActivityTest ActiveDispatcherTest \ NotificationsTestSuite NullStreamTest NumberFormatterTest \ NumberParserTest PathTest PatternFormatterTest PBKDF2EngineTest RWLockTest \ RandomStreamTest RandomTest RegularExpressionTest SHA1EngineTest \ - SemaphoreTest ConditionTest SharedLibraryTest SharedLibraryTestSuite \ + SemaphoreTest MutexTest ConditionTest SharedLibraryTest SharedLibraryTestSuite \ SimpleFileChannelTest StopwatchTest \ StreamConverterTest StreamCopierTest StreamTokenizerTest \ StreamsTestSuite StringTest StringTokenizerTest TaskTestSuite TaskTest \ diff --git a/Foundation/testsuite/TestSuite.vxbuild b/Foundation/testsuite/TestSuite.vxbuild index f6bc668d3..973cc8671 100644 --- a/Foundation/testsuite/TestSuite.vxbuild +++ b/Foundation/testsuite/TestSuite.vxbuild @@ -66,6 +66,7 @@ SOURCES=" ManifestTest.cpp MemoryPoolTest.cpp MemoryStreamTest.cpp + MutexTest.cpp NDCTest.cpp NotificationCenterTest.cpp NotificationQueueTest.cpp diff --git a/Foundation/testsuite/TestSuite_CE_vs90.vcproj b/Foundation/testsuite/TestSuite_CE_vs90.vcproj index ec965cc18..133be0ef7 100644 --- a/Foundation/testsuite/TestSuite_CE_vs90.vcproj +++ b/Foundation/testsuite/TestSuite_CE_vs90.vcproj @@ -1074,6 +1074,10 @@ RelativePath=".\src\ConditionTest.cpp" > + + @@ -1122,6 +1126,10 @@ RelativePath=".\src\ConditionTest.h" > + + diff --git a/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj b/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj index ef0272349..6a61bb9ab 100644 --- a/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj +++ b/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj @@ -340,6 +340,7 @@ + @@ -477,6 +478,7 @@ + diff --git a/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj.filters b/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj.filters index 09c1055a8..d6753d31a 100644 --- a/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_WEC2013_vs110.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -746,6 +749,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj b/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj index bf0f80d9c..adf82a629 100644 --- a/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj +++ b/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj @@ -343,6 +343,7 @@ + @@ -480,6 +481,7 @@ + diff --git a/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj.filters b/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj.filters index 09c1055a8..d6753d31a 100644 --- a/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_WEC2013_vs120.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -746,6 +749,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_vs100.vcxproj b/Foundation/testsuite/TestSuite_vs100.vcxproj index 5c00215c6..4a6303364 100644 --- a/Foundation/testsuite/TestSuite_vs100.vcxproj +++ b/Foundation/testsuite/TestSuite_vs100.vcxproj @@ -360,6 +360,7 @@ + @@ -498,6 +499,7 @@ + diff --git a/Foundation/testsuite/TestSuite_vs100.vcxproj.filters b/Foundation/testsuite/TestSuite_vs100.vcxproj.filters index 3d1e13b6c..448b4375a 100644 --- a/Foundation/testsuite/TestSuite_vs100.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_vs100.vcxproj.filters @@ -330,6 +330,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -740,6 +743,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_vs110.vcxproj b/Foundation/testsuite/TestSuite_vs110.vcxproj index 12af3746f..ce4a95364 100644 --- a/Foundation/testsuite/TestSuite_vs110.vcxproj +++ b/Foundation/testsuite/TestSuite_vs110.vcxproj @@ -365,6 +365,7 @@ + @@ -503,6 +504,7 @@ + diff --git a/Foundation/testsuite/TestSuite_vs110.vcxproj.filters b/Foundation/testsuite/TestSuite_vs110.vcxproj.filters index 5826f6b50..da77db8bd 100644 --- a/Foundation/testsuite/TestSuite_vs110.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_vs110.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -746,6 +749,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_vs120.vcxproj b/Foundation/testsuite/TestSuite_vs120.vcxproj index 7b9b07f68..184c63e2a 100644 --- a/Foundation/testsuite/TestSuite_vs120.vcxproj +++ b/Foundation/testsuite/TestSuite_vs120.vcxproj @@ -388,6 +388,7 @@ + @@ -525,6 +526,7 @@ + diff --git a/Foundation/testsuite/TestSuite_vs120.vcxproj.filters b/Foundation/testsuite/TestSuite_vs120.vcxproj.filters index 57dee76b7..f8a87ea2c 100644 --- a/Foundation/testsuite/TestSuite_vs120.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_vs120.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -743,6 +746,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_vs71.vcproj b/Foundation/testsuite/TestSuite_vs71.vcproj index c9c589105..a5cf195cc 100644 --- a/Foundation/testsuite/TestSuite_vs71.vcproj +++ b/Foundation/testsuite/TestSuite_vs71.vcproj @@ -775,6 +775,9 @@ + + @@ -812,6 +815,9 @@ + + diff --git a/Foundation/testsuite/TestSuite_vs80.vcproj b/Foundation/testsuite/TestSuite_vs80.vcproj index 369511176..9931f9202 100644 --- a/Foundation/testsuite/TestSuite_vs80.vcproj +++ b/Foundation/testsuite/TestSuite_vs80.vcproj @@ -1043,6 +1043,10 @@ RelativePath=".\src\ConditionTest.cpp" > + + @@ -1091,6 +1095,10 @@ RelativePath=".\src\ConditionTest.h" > + + diff --git a/Foundation/testsuite/TestSuite_vs90.vcproj b/Foundation/testsuite/TestSuite_vs90.vcproj index ce71ad1b5..f49443a71 100644 --- a/Foundation/testsuite/TestSuite_vs90.vcproj +++ b/Foundation/testsuite/TestSuite_vs90.vcproj @@ -1029,6 +1029,10 @@ RelativePath=".\src\ConditionTest.cpp" > + + @@ -1077,6 +1081,10 @@ RelativePath=".\src\ConditionTest.h" > + + diff --git a/Foundation/testsuite/TestSuite_x64_vs100.vcxproj b/Foundation/testsuite/TestSuite_x64_vs100.vcxproj index c0dd320a8..4d1533133 100644 --- a/Foundation/testsuite/TestSuite_x64_vs100.vcxproj +++ b/Foundation/testsuite/TestSuite_x64_vs100.vcxproj @@ -359,6 +359,7 @@ + @@ -498,6 +499,7 @@ + diff --git a/Foundation/testsuite/TestSuite_x64_vs100.vcxproj.filters b/Foundation/testsuite/TestSuite_x64_vs100.vcxproj.filters index 731605748..95fa60b22 100644 --- a/Foundation/testsuite/TestSuite_x64_vs100.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_x64_vs100.vcxproj.filters @@ -339,6 +339,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -752,6 +755,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_x64_vs110.vcxproj b/Foundation/testsuite/TestSuite_x64_vs110.vcxproj index d6f2f49bc..d4925a7a2 100644 --- a/Foundation/testsuite/TestSuite_x64_vs110.vcxproj +++ b/Foundation/testsuite/TestSuite_x64_vs110.vcxproj @@ -365,6 +365,7 @@ + @@ -504,6 +505,7 @@ + diff --git a/Foundation/testsuite/TestSuite_x64_vs110.vcxproj.filters b/Foundation/testsuite/TestSuite_x64_vs110.vcxproj.filters index 6a1fd5330..e93991ff3 100644 --- a/Foundation/testsuite/TestSuite_x64_vs110.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_x64_vs110.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -749,6 +752,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_x64_vs120.vcxproj b/Foundation/testsuite/TestSuite_x64_vs120.vcxproj index af0a446fa..5042d647f 100644 --- a/Foundation/testsuite/TestSuite_x64_vs120.vcxproj +++ b/Foundation/testsuite/TestSuite_x64_vs120.vcxproj @@ -372,6 +372,7 @@ + @@ -510,6 +511,7 @@ + diff --git a/Foundation/testsuite/TestSuite_x64_vs120.vcxproj.filters b/Foundation/testsuite/TestSuite_x64_vs120.vcxproj.filters index ebd700818..f1072639f 100644 --- a/Foundation/testsuite/TestSuite_x64_vs120.vcxproj.filters +++ b/Foundation/testsuite/TestSuite_x64_vs120.vcxproj.filters @@ -336,6 +336,9 @@ Threading\Source Files + + Threading\Source Files + Threading\Source Files @@ -749,6 +752,9 @@ Threading\Header Files + + Threading\Header Files + Threading\Header Files diff --git a/Foundation/testsuite/TestSuite_x64_vs90.vcproj b/Foundation/testsuite/TestSuite_x64_vs90.vcproj index 704ac1cf2..49979b696 100644 --- a/Foundation/testsuite/TestSuite_x64_vs90.vcproj +++ b/Foundation/testsuite/TestSuite_x64_vs90.vcproj @@ -1023,6 +1023,10 @@ RelativePath=".\src\ConditionTest.cpp" > + + @@ -1071,6 +1075,10 @@ RelativePath=".\src\ConditionTest.h" > + + diff --git a/Foundation/testsuite/src/MutexTest.cpp b/Foundation/testsuite/src/MutexTest.cpp new file mode 100644 index 000000000..3bab661b6 --- /dev/null +++ b/Foundation/testsuite/src/MutexTest.cpp @@ -0,0 +1,122 @@ +// +// MutexTest.cpp +// +// $Id: //poco/1.4/Foundation/testsuite/src/MutexTest.cpp#1 $ +// +// Copyright (c) 2007, Applied Informatics Software Engineering GmbH. +// and Contributors. +// +// SPDX-License-Identifier: BSL-1.0 +// + + +#include "MutexTest.h" +#include "CppUnit/TestCaller.h" +#include "CppUnit/TestSuite.h" +#include "Poco/Exception.h" +#include "Poco/Mutex.h" +#include "Poco/Runnable.h" +#include "Poco/Thread.h" +#include "Poco/Timestamp.h" + + +using Poco::FastMutex; +using Poco::Runnable; +using Poco::SystemException; +using Poco::Thread; +using Poco::Timestamp; + +namespace +{ + class SelfDeadlockRunnable: public Runnable + { + public: + SelfDeadlockRunnable(): + _ran(false) + { + } + + void run() + { + try + { + FastMutex mtx; + mtx.lock(); + mtx.lock(); + } + catch (...) + { + } + + _ran = true; + } + + bool ran() const + { + return _ran; + } + + private: + bool _ran; + }; +} + + +MutexTest::MutexTest(const std::string& name): CppUnit::TestCase(name) +{ +} + + +MutexTest::~MutexTest() +{ +} + + +void MutexTest::testFastMutexRecursion() +{ + FastMutex mtx; + mtx.lock(); + + bool success = mtx.tryLock(); + assert (!success); + + Timestamp mark; + success = mtx.tryLock(2000); // Wait 2 seconds + assert (!success); + + // Test that we waited approx. 2 sec + Timestamp::TimeDiff elapsed = mark.elapsed(); + assert (elapsed > 1000000); + assert (elapsed < 3000000); + + success = mtx.tryLock(0); + assert (!success); + + SelfDeadlockRunnable r; + Thread t; + + t.start(r); + success = t.tryJoin(2000); + assert (!success); + assert (!r.ran()); +} + + +void MutexTest::setUp() +{ +} + + +void MutexTest::tearDown() +{ +} + + +CppUnit::Test* MutexTest::suite() +{ + CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("MutexTest"); + + CppUnit_addTest(pSuite, MutexTest, testFastMutexRecursion); + + return pSuite; +} diff --git a/Foundation/testsuite/src/MutexTest.h b/Foundation/testsuite/src/MutexTest.h new file mode 100644 index 000000000..3d3a5586c --- /dev/null +++ b/Foundation/testsuite/src/MutexTest.h @@ -0,0 +1,40 @@ +// +// MutexTest.h +// +// $Id: //poco/1.4/Foundation/testsuite/src/MutexTest.h#1 $ +// +// Definition of the MutexTest class. +// +// Copyright (c) 2007, Applied Informatics Software Engineering GmbH. +// and Contributors. +// +// SPDX-License-Identifier: BSL-1.0 +// + + +#ifndef MutexTest_INCLUDED +#define MutexTest_INCLUDED + + +#include "Poco/Foundation.h" +#include "CppUnit/TestCase.h" + + +class MutexTest: public CppUnit::TestCase +{ +public: + MutexTest(const std::string& name); + ~MutexTest(); + + void testFastMutexRecursion(); + + void setUp(); + void tearDown(); + + static CppUnit::Test* suite(); + +private: +}; + + +#endif // MutexTest_INCLUDED diff --git a/Foundation/testsuite/src/ThreadingTestSuite.cpp b/Foundation/testsuite/src/ThreadingTestSuite.cpp index 481396b8f..f06d470c0 100644 --- a/Foundation/testsuite/src/ThreadingTestSuite.cpp +++ b/Foundation/testsuite/src/ThreadingTestSuite.cpp @@ -12,6 +12,7 @@ #include "ThreadingTestSuite.h" #include "ThreadTest.h" +#include "MutexTest.h" #include "SemaphoreTest.h" #include "RWLockTest.h" #include "ThreadPoolTest.h" @@ -28,6 +29,7 @@ CppUnit::Test* ThreadingTestSuite::suite() CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("ThreadingTestSuite"); pSuite->addTest(ThreadTest::suite()); + pSuite->addTest(MutexTest::suite()); pSuite->addTest(SemaphoreTest::suite()); pSuite->addTest(RWLockTest::suite()); pSuite->addTest(ThreadPoolTest::suite()); From 120fd4433cbb31e683f5879ab60e07098986c1b7 Mon Sep 17 00:00:00 2001 From: martin-osborne Date: Sun, 19 Oct 2014 18:11:43 +0100 Subject: [PATCH 2/2] Simplified the timed try lock implementation. --- Foundation/src/Mutex_WIN32.cpp | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/Foundation/src/Mutex_WIN32.cpp b/Foundation/src/Mutex_WIN32.cpp index 5700cc35b..5dda34e1f 100644 --- a/Foundation/src/Mutex_WIN32.cpp +++ b/Foundation/src/Mutex_WIN32.cpp @@ -128,19 +128,8 @@ bool FastMutexImpl::tryLockImpl(long milliseconds) { try { - if (TryEnterCriticalSection(&_cs) == TRUE) - { - if (_lockCount == 0) - { - ++_lockCount; - return true; - } - - // We're trying to go recursive - LeaveCriticalSection(&_cs); - Sleep(milliseconds); - return false; - } + if (tryLockImpl()) + return true; } catch (...) {