From de6ef5c6955684f89c269b12c64de731a8f54fb9 Mon Sep 17 00:00:00 2001 From: marcosb Date: Mon, 10 Feb 2020 13:39:34 -0700 Subject: [PATCH 01/13] Remove manual locking/unlocking --- src/inet/AsyncDNSResolverSockets.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/inet/AsyncDNSResolverSockets.h b/src/inet/AsyncDNSResolverSockets.h index 9e0550d103..37a368e48b 100644 --- a/src/inet/AsyncDNSResolverSockets.h +++ b/src/inet/AsyncDNSResolverSockets.h @@ -88,10 +88,6 @@ class AsyncDNSResolverSockets static void *AsyncDNSThreadRun(void *args); static void NotifyWeaveThread(DNSResolver *resolver); - - void AsyncMutexLock(void); - - void AsyncMutexUnlock(void); }; } // namespace Inet From 00ef599b3a432a361eaaf0c991b0f025b51c2fa9 Mon Sep 17 00:00:00 2001 From: marcosb Date: Mon, 10 Feb 2020 13:46:05 -0700 Subject: [PATCH 02/13] Use RAII for lock release --- src/inet/AsyncDNSResolverSockets.cpp | 71 +++++++++++++--------------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index dabe0dba34..f20b679f71 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -36,6 +36,29 @@ namespace nl { namespace Inet { + +class LockHolder { + public: + LockHolder(pthread_mutex_t ayncDNSMutex) + :mAsyncDNSMutex(ayncDNSMutex) + { + int pthreadErr; + + pthreadErr = pthread_mutex_lock(&mAsyncDNSMutex); + VerifyOrDie(pthreadErr == 0); + } + + ~LockHolder() + { + int pthreadErr; + + pthreadErr = pthread_mutex_unlock(&mAsyncDNSMutex); + VerifyOrDie(pthreadErr == 0); + } + + private: + pthread_mutex_t mAsyncDNSMutex; +} /** * The explicit initializer for the AsynchronousDNSResolverSockets class. @@ -87,14 +110,14 @@ INET_ERROR AsyncDNSResolverSockets::Shutdown(void) INET_ERROR err = INET_NO_ERROR; int pthreadErr; - AsyncMutexLock(); - - mInet->State = InetLayer::kState_ShutdownInProgress; + { + LockHolder asyncMutexLock(mAsyncDNSMutex); - pthreadErr = pthread_cond_broadcast(&mAsyncDNSCondVar); - VerifyOrDie(pthreadErr == 0); + mInet->State = InetLayer::kState_ShutdownInProgress; - AsyncMutexUnlock(); + pthreadErr = pthread_cond_broadcast(&mAsyncDNSCondVar); + VerifyOrDie(pthreadErr == 0); + } // Have the Weave thread join the thread pool for asynchronous DNS resolution. for (int i = 0; i < INET_CONFIG_DNS_ASYNC_MAX_THREAD_COUNT; i++) @@ -172,9 +195,8 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver) { INET_ERROR err = INET_NO_ERROR; int pthreadErr; - - AsyncMutexLock(); - + LockHolder asyncMutexLock(mAsyncDNSMutex); + // Add the DNSResolver object to the queue. if (mAsyncDNSQueueHead == NULL) { @@ -191,8 +213,6 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver) pthreadErr = pthread_cond_signal(&mAsyncDNSCondVar); VerifyOrDie(pthreadErr == 0); - AsyncMutexUnlock(); - return err; } @@ -205,8 +225,7 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver) { INET_ERROR err = INET_NO_ERROR; int pthreadErr; - - AsyncMutexLock(); + LockHolder asyncMutexLock(mAsyncDNSMutex); // block until there is work to do or we detect a shutdown while ( (mAsyncDNSQueueHead == NULL) && @@ -236,8 +255,6 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver) } } - AsyncMutexUnlock(); - return err; } @@ -249,8 +266,7 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver) INET_ERROR AsyncDNSResolverSockets::Cancel(DNSResolver &resolver) { INET_ERROR err = INET_NO_ERROR; - - AsyncMutexLock(); + LockHolder asyncMutexLock(mAsyncDNSMutex); resolver.mState = DNSResolver::kState_Canceled; @@ -282,7 +298,7 @@ void AsyncDNSResolverSockets::Resolve(DNSResolver &resolver) gaiReturnCode = getaddrinfo(resolver.asyncHostNameBuf, NULL, &gaiHints, &gaiResults); // Mutex protects the read and write operation on resolver->mState - AsyncMutexLock(); + LockHolder asyncMutexLock(mAsyncDNSMutex); // Process the return code and results list returned by getaddrinfo(). If the call // was successful this will copy the resultant addresses into the caller's array. @@ -291,9 +307,6 @@ void AsyncDNSResolverSockets::Resolve(DNSResolver &resolver) // Set the DNS resolver state. resolver.mState = DNSResolver::kState_Complete; - // Release lock. - AsyncMutexUnlock(); - return; } @@ -348,22 +361,6 @@ void *AsyncDNSResolverSockets::AsyncDNSThreadRun(void *args) return NULL; } -void AsyncDNSResolverSockets::AsyncMutexLock(void) -{ - int pthreadErr; - - pthreadErr = pthread_mutex_lock(&mAsyncDNSMutex); - VerifyOrDie(pthreadErr == 0); -} - -void AsyncDNSResolverSockets::AsyncMutexUnlock(void) -{ - int pthreadErr; - - pthreadErr = pthread_mutex_unlock(&mAsyncDNSMutex); - VerifyOrDie(pthreadErr == 0); -} - } // namespace Inet } // namespace nl #endif // INET_CONFIG_ENABLE_DNS_RESOLVER && INET_CONFIG_ENABLE_ASYNC_DNS_SOCKETS From 25827d5f4846538d320fa8d1dfc9750aa6358af7 Mon Sep 17 00:00:00 2001 From: marcosb Date: Mon, 10 Feb 2020 15:31:08 -0700 Subject: [PATCH 03/13] Fix some errors post-compile --- src/inet/AsyncDNSResolverSockets.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index f20b679f71..881819dfdf 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -36,11 +36,11 @@ namespace nl { namespace Inet { - + class LockHolder { public: - LockHolder(pthread_mutex_t ayncDNSMutex) - :mAsyncDNSMutex(ayncDNSMutex) + LockHolder(pthread_mutex_t asyncDNSMutex) + :mAsyncDNSMutex(asyncDNSMutex) { int pthreadErr; @@ -55,10 +55,10 @@ class LockHolder { pthreadErr = pthread_mutex_unlock(&mAsyncDNSMutex); VerifyOrDie(pthreadErr == 0); } - + private: pthread_mutex_t mAsyncDNSMutex; -} +}; /** * The explicit initializer for the AsynchronousDNSResolverSockets class. @@ -196,7 +196,7 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver) INET_ERROR err = INET_NO_ERROR; int pthreadErr; LockHolder asyncMutexLock(mAsyncDNSMutex); - + // Add the DNSResolver object to the queue. if (mAsyncDNSQueueHead == NULL) { @@ -270,8 +270,6 @@ INET_ERROR AsyncDNSResolverSockets::Cancel(DNSResolver &resolver) resolver.mState = DNSResolver::kState_Canceled; - AsyncMutexUnlock(); - return err; } From aa70426c9b5e3861f428f5f05d9083ffb56ec9f0 Mon Sep 17 00:00:00 2001 From: marcosb Date: Tue, 11 Feb 2020 11:11:20 -0700 Subject: [PATCH 04/13] By-reference passing is possibly changing pthread_mutex_t & causing deadlocks; see if this fixes it --- src/inet/AsyncDNSResolverSockets.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index 881819dfdf..587f8128a4 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -57,7 +57,7 @@ class LockHolder { } private: - pthread_mutex_t mAsyncDNSMutex; + pthread_mutex_t& mAsyncDNSMutex; }; /** From 9ed08b255c60250bf9b597eefcb2b93d88b73360 Mon Sep 17 00:00:00 2001 From: marcosb Date: Tue, 11 Feb 2020 11:11:43 -0700 Subject: [PATCH 05/13] Missed param ref --- src/inet/AsyncDNSResolverSockets.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index 587f8128a4..c96806b49b 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -39,7 +39,7 @@ namespace Inet { class LockHolder { public: - LockHolder(pthread_mutex_t asyncDNSMutex) + LockHolder(pthread_mutex_t& asyncDNSMutex) :mAsyncDNSMutex(asyncDNSMutex) { int pthreadErr; From 000a78142e4be98d365e8cc1379aef066e809c9a Mon Sep 17 00:00:00 2001 From: marcosb Date: Tue, 11 Feb 2020 11:15:30 -0700 Subject: [PATCH 06/13] Use direct reference to class --- src/inet/AsyncDNSResolverSockets.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index c96806b49b..4f9b354094 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -39,12 +39,12 @@ namespace Inet { class LockHolder { public: - LockHolder(pthread_mutex_t& asyncDNSMutex) - :mAsyncDNSMutex(asyncDNSMutex) + LockHolder(AsyncDNSResolverSockets& resolver) + :mResolver(resolver) { int pthreadErr; - pthreadErr = pthread_mutex_lock(&mAsyncDNSMutex); + pthreadErr = pthread_mutex_lock(&resolver.mAsyncDNSMutex); VerifyOrDie(pthreadErr == 0); } @@ -52,12 +52,12 @@ class LockHolder { { int pthreadErr; - pthreadErr = pthread_mutex_unlock(&mAsyncDNSMutex); + pthreadErr = pthread_mutex_unlock(&resolver.mAsyncDNSMutex); VerifyOrDie(pthreadErr == 0); } private: - pthread_mutex_t& mAsyncDNSMutex; + AsyncDNSResolverSockets& mResolver; }; /** @@ -111,7 +111,7 @@ INET_ERROR AsyncDNSResolverSockets::Shutdown(void) int pthreadErr; { - LockHolder asyncMutexLock(mAsyncDNSMutex); + LockHolder asyncMutexLock(*this); mInet->State = InetLayer::kState_ShutdownInProgress; @@ -195,7 +195,7 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver) { INET_ERROR err = INET_NO_ERROR; int pthreadErr; - LockHolder asyncMutexLock(mAsyncDNSMutex); + LockHolder asyncMutexLock(*this); // Add the DNSResolver object to the queue. if (mAsyncDNSQueueHead == NULL) @@ -225,7 +225,7 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver) { INET_ERROR err = INET_NO_ERROR; int pthreadErr; - LockHolder asyncMutexLock(mAsyncDNSMutex); + LockHolder asyncMutexLock(*this); // block until there is work to do or we detect a shutdown while ( (mAsyncDNSQueueHead == NULL) && @@ -266,7 +266,7 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver) INET_ERROR AsyncDNSResolverSockets::Cancel(DNSResolver &resolver) { INET_ERROR err = INET_NO_ERROR; - LockHolder asyncMutexLock(mAsyncDNSMutex); + LockHolder asyncMutexLock(*this); resolver.mState = DNSResolver::kState_Canceled; @@ -296,7 +296,7 @@ void AsyncDNSResolverSockets::Resolve(DNSResolver &resolver) gaiReturnCode = getaddrinfo(resolver.asyncHostNameBuf, NULL, &gaiHints, &gaiResults); // Mutex protects the read and write operation on resolver->mState - LockHolder asyncMutexLock(mAsyncDNSMutex); + LockHolder asyncMutexLock(*this); // Process the return code and results list returned by getaddrinfo(). If the call // was successful this will copy the resultant addresses into the caller's array. From a3e7c4e565a06f8c2b6db1af7a1478dfac5b9d4c Mon Sep 17 00:00:00 2001 From: marcosb Date: Tue, 11 Feb 2020 11:16:13 -0700 Subject: [PATCH 07/13] Add LockHolder as friend for direct lock access --- src/inet/AsyncDNSResolverSockets.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/inet/AsyncDNSResolverSockets.h b/src/inet/AsyncDNSResolverSockets.h index 37a368e48b..cab217d096 100644 --- a/src/inet/AsyncDNSResolverSockets.h +++ b/src/inet/AsyncDNSResolverSockets.h @@ -54,6 +54,7 @@ class AsyncDNSResolverSockets { friend class InetLayer; friend class DNSResolver; + friend class LockHolder; public: INET_ERROR EnqueueRequest(DNSResolver &resolver); From fce262511d3072d9b31e96d221c2e1a30caa1c8e Mon Sep 17 00:00:00 2001 From: marcosb Date: Tue, 11 Feb 2020 11:48:10 -0700 Subject: [PATCH 08/13] Update member field name --- src/inet/AsyncDNSResolverSockets.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/inet/AsyncDNSResolverSockets.cpp b/src/inet/AsyncDNSResolverSockets.cpp index 4f9b354094..cdde16627f 100644 --- a/src/inet/AsyncDNSResolverSockets.cpp +++ b/src/inet/AsyncDNSResolverSockets.cpp @@ -44,7 +44,7 @@ class LockHolder { { int pthreadErr; - pthreadErr = pthread_mutex_lock(&resolver.mAsyncDNSMutex); + pthreadErr = pthread_mutex_lock(&mResolver.mAsyncDNSMutex); VerifyOrDie(pthreadErr == 0); } @@ -52,7 +52,7 @@ class LockHolder { { int pthreadErr; - pthreadErr = pthread_mutex_unlock(&resolver.mAsyncDNSMutex); + pthreadErr = pthread_mutex_unlock(&mResolver.mAsyncDNSMutex); VerifyOrDie(pthreadErr == 0); } From 397a55aff97bea6e5c47c34965495f4aca7810ed Mon Sep 17 00:00:00 2001 From: marcosb Date: Wed, 12 Feb 2020 11:15:06 -0700 Subject: [PATCH 09/13] AutoRelease for Object --- src/system/SystemObject.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/system/SystemObject.h b/src/system/SystemObject.h index 00e0377391..7f26877942 100644 --- a/src/system/SystemObject.h +++ b/src/system/SystemObject.h @@ -81,6 +81,17 @@ class NL_DLL_EXPORT Object void Retain(void); void Release(void); Layer& SystemLayer(void) const; + + class AutoRelease { + public: + AutoRelease(Object& owner) : mOwner(owner) {} + ~AutoRelease(void) { + owner.Release(); + } + + private: + Object& mOwner; + } protected: #if WEAVE_SYSTEM_CONFIG_USE_LWIP From 2b1c818ee7dd758cf06efcd93b220aa3888fb3df Mon Sep 17 00:00:00 2001 From: marcosb Date: Wed, 12 Feb 2020 11:18:40 -0700 Subject: [PATCH 10/13] Use AutoRelease instead of manually calling Release() --- src/inet/DNSResolver.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/inet/DNSResolver.cpp b/src/inet/DNSResolver.cpp index 126941ebc7..9612026a96 100644 --- a/src/inet/DNSResolver.cpp +++ b/src/inet/DNSResolver.cpp @@ -91,9 +91,9 @@ INET_ERROR DNSResolver::Resolve(const char *hostName, uint16_t hostNameLen, uint DNSResolver::OnResolveCompleteFunct onComplete, void *appState) { INET_ERROR res = INET_NO_ERROR; + Weave::System::Object::AutoRelease objectRelease(*this); #if !WEAVE_SYSTEM_CONFIG_USE_SOCKETS && !LWIP_DNS - Release(); return INET_ERROR_NOT_IMPLEMENTED; #endif // !WEAVE_SYSTEM_CONFIG_USE_SOCKETS && !LWIP_DNS @@ -108,7 +108,6 @@ INET_ERROR DNSResolver::Resolve(const char *hostName, uint16_t hostNameLen, uint addrFamilyOption != kDNSOption_AddrFamily_IPv6Preferred) || (optionFlags & ~kDNSOption_ValidFlags) != 0) { - Release(); return INET_ERROR_BAD_ARGS; } @@ -164,7 +163,6 @@ INET_ERROR DNSResolver::Resolve(const char *hostName, uint16_t hostNameLen, uint if (addrFamilyOption == kDNSOption_AddrFamily_IPv6Only) #endif { - Release(); return INET_ERROR_NOT_IMPLEMENTED; } @@ -202,7 +200,6 @@ INET_ERROR DNSResolver::Resolve(const char *hostName, uint16_t hostNameLen, uint else if (lwipErr != ERR_INPROGRESS) { res = Weave::System::MapErrorLwIP(lwipErr); - Release(); } return res; @@ -228,9 +225,6 @@ INET_ERROR DNSResolver::Resolve(const char *hostName, uint16_t hostNameLen, uint // Invoke the caller's completion function. onComplete(appState, res, NumAddrs, addrArray); - // Release DNSResolver object. - Release(); - return INET_NO_ERROR; #endif // WEAVE_SYSTEM_CONFIG_USE_SOCKETS @@ -298,12 +292,11 @@ INET_ERROR DNSResolver::Cancel() */ void DNSResolver::HandleResolveComplete() { + Weave::System::Object::AutoRelease objectRelease(*this); + // Call the application's completion handler if the request hasn't been canceled. if (OnComplete != NULL) OnComplete(AppState, (NumAddrs > 0) ? INET_NO_ERROR : INET_ERROR_HOST_NOT_FOUND, NumAddrs, AddrArray); - - // Release the resolver object. - Release(); } @@ -530,13 +523,13 @@ uint8_t DNSResolver::CountAddresses(int family, const struct addrinfo * addrs) void DNSResolver::HandleAsyncResolveComplete(void) { + Weave::System::Object::AutoRelease objectRelease(*this); + // Copy the resolved address to the application supplied buffer, but only if the request hasn't been canceled. if (OnComplete && mState != kState_Canceled) { OnComplete(AppState, asyncDNSResolveResult, NumAddrs, AddrArray); } - - Release(); } #endif // INET_CONFIG_ENABLE_ASYNC_DNS_SOCKETS #endif // WEAVE_SYSTEM_CONFIG_USE_SOCKETS From 9d999fd4feb77108a447641370021a0b7a78ef1d Mon Sep 17 00:00:00 2001 From: marcosb Date: Wed, 12 Feb 2020 11:44:57 -0700 Subject: [PATCH 11/13] Fix compilation --- src/system/SystemObject.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/system/SystemObject.h b/src/system/SystemObject.h index 7f26877942..07427618d2 100644 --- a/src/system/SystemObject.h +++ b/src/system/SystemObject.h @@ -86,12 +86,12 @@ class NL_DLL_EXPORT Object public: AutoRelease(Object& owner) : mOwner(owner) {} ~AutoRelease(void) { - owner.Release(); + mOwner.Release(); } private: Object& mOwner; - } + }; protected: #if WEAVE_SYSTEM_CONFIG_USE_LWIP From d7fc131d8e984406ff44791b54ed1eddfa7e4ed0 Mon Sep 17 00:00:00 2001 From: marcosb Date: Fri, 14 Feb 2020 10:52:32 -0700 Subject: [PATCH 12/13] Stylistic fixes --- src/system/SystemObject.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/system/SystemObject.h b/src/system/SystemObject.h index 07427618d2..5d5171ca91 100644 --- a/src/system/SystemObject.h +++ b/src/system/SystemObject.h @@ -81,11 +81,15 @@ class NL_DLL_EXPORT Object void Retain(void); void Release(void); Layer& SystemLayer(void) const; - + class AutoRelease { public: - AutoRelease(Object& owner) : mOwner(owner) {} - ~AutoRelease(void) { + AutoRelease(Object& owner) : mOwner(owner) + { + } + + ~AutoRelease(void) + { mOwner.Release(); } From 749856a87fe0570a1fc1104574aed63cac0a0a2e Mon Sep 17 00:00:00 2001 From: marcosb Date: Fri, 14 Feb 2020 14:04:11 -0700 Subject: [PATCH 13/13] Stylistic fixes --- src/system/SystemObject.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/system/SystemObject.h b/src/system/SystemObject.h index 5d5171ca91..8295705b41 100644 --- a/src/system/SystemObject.h +++ b/src/system/SystemObject.h @@ -87,12 +87,12 @@ class NL_DLL_EXPORT Object AutoRelease(Object& owner) : mOwner(owner) { } - + ~AutoRelease(void) { mOwner.Release(); } - + private: Object& mOwner; };