Skip to content
This repository has been archived by the owner on Dec 20, 2023. It is now read-only.

New to openweave #492

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

New to openweave #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
de6ef5c
Remove manual locking/unlocking
marcosb Feb 10, 2020
00ef599
Use RAII for lock release
marcosb Feb 10, 2020
25827d5
Fix some errors post-compile
marcosb Feb 10, 2020
aa70426
By-reference passing is possibly changing pthread_mutex_t & causing d…
marcosb Feb 11, 2020
9ed08b2
Missed param ref
marcosb Feb 11, 2020
000a781
Use direct reference to class
marcosb Feb 11, 2020
a3e7c4e
Add LockHolder as friend for direct lock access
marcosb Feb 11, 2020
fce2625
Update member field name
marcosb Feb 11, 2020
397a55a
AutoRelease for Object
marcosb Feb 12, 2020
2b1c818
Use AutoRelease instead of manually calling Release()
marcosb Feb 12, 2020
9d999fd
Fix compilation
marcosb Feb 12, 2020
d7fc131
Stylistic fixes
marcosb Feb 14, 2020
749856a
Stylistic fixes
marcosb Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 33 additions & 38 deletions src/inet/AsyncDNSResolverSockets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,29 @@
namespace nl {
namespace Inet {

class LockHolder {
public:
LockHolder(AsyncDNSResolverSockets& resolver)
:mResolver(resolver)
{
int pthreadErr;

pthreadErr = pthread_mutex_lock(&mResolver.mAsyncDNSMutex);
VerifyOrDie(pthreadErr == 0);
}

~LockHolder()
{
int pthreadErr;

pthreadErr = pthread_mutex_unlock(&mResolver.mAsyncDNSMutex);
VerifyOrDie(pthreadErr == 0);
}

private:
AsyncDNSResolverSockets& mResolver;
};

/**
* The explicit initializer for the AsynchronousDNSResolverSockets class.
* This initializes the mutex and semaphore variables and creates the
Expand Down Expand Up @@ -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(*this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This illustrates one of the downsides of this pattern--the point at which the lock is acquired doesn't read as an imperative statement, and hence is easily missed by the unaware reader.

I think a construct like this would be more obvious:

{
    auto lock = LockHolder::AquireLock(*this);

    ...
}


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++)
Expand Down Expand Up @@ -172,8 +195,7 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver)
{
INET_ERROR err = INET_NO_ERROR;
int pthreadErr;

AsyncMutexLock();
LockHolder asyncMutexLock(*this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, I think this pattern makes it easy to miss the acquisition of the lock. I suggest always using an independent block to clearly demark the scope of the lock, with the lock being the only variable declared at the start of the block. E.g.:

INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver)
{
    INET_ERROR err = INET_NO_ERROR;
    int pthreadErr;

    {
        auto lock = LockHolder::AquireLock(*this);

        ...
    }

    return err;
}


// Add the DNSResolver object to the queue.
if (mAsyncDNSQueueHead == NULL)
Expand All @@ -191,8 +213,6 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver)
pthreadErr = pthread_cond_signal(&mAsyncDNSCondVar);
VerifyOrDie(pthreadErr == 0);

AsyncMutexUnlock();

return err;
}

Expand All @@ -205,8 +225,7 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver)
{
INET_ERROR err = INET_NO_ERROR;
int pthreadErr;

AsyncMutexLock();
LockHolder asyncMutexLock(*this);

// block until there is work to do or we detect a shutdown
while ( (mAsyncDNSQueueHead == NULL) &&
Expand Down Expand Up @@ -236,8 +255,6 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver)
}
}

AsyncMutexUnlock();

return err;
}

Expand All @@ -249,13 +266,10 @@ INET_ERROR AsyncDNSResolverSockets::DequeueRequest(DNSResolver **outResolver)
INET_ERROR AsyncDNSResolverSockets::Cancel(DNSResolver &resolver)
{
INET_ERROR err = INET_NO_ERROR;

AsyncMutexLock();
LockHolder asyncMutexLock(*this);

resolver.mState = DNSResolver::kState_Canceled;

AsyncMutexUnlock();

return err;
}

Expand All @@ -282,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
AsyncMutexLock();
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.
Expand All @@ -291,9 +305,6 @@ void AsyncDNSResolverSockets::Resolve(DNSResolver &resolver)
// Set the DNS resolver state.
resolver.mState = DNSResolver::kState_Complete;

// Release lock.
AsyncMutexUnlock();

return;
}

Expand Down Expand Up @@ -348,22 +359,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
Expand Down
5 changes: 1 addition & 4 deletions src/inet/AsyncDNSResolverSockets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class AsyncDNSResolverSockets
{
friend class InetLayer;
friend class DNSResolver;
friend class LockHolder;
public:

INET_ERROR EnqueueRequest(DNSResolver &resolver);
Expand Down Expand Up @@ -88,10 +89,6 @@ class AsyncDNSResolverSockets
static void *AsyncDNSThreadRun(void *args);

static void NotifyWeaveThread(DNSResolver *resolver);

void AsyncMutexLock(void);

void AsyncMutexUnlock(void);
};

} // namespace Inet
Expand Down
17 changes: 5 additions & 12 deletions src/inet/DNSResolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment below, I don't see this as a desirable pattern. Rather, I would argue that the returns should be replaced with ExitNow() jumps to an exit: label containing a single call to Release().


#if !WEAVE_SYSTEM_CONFIG_USE_SOCKETS && !LWIP_DNS
Release();
return INET_ERROR_NOT_IMPLEMENTED;
#endif // !WEAVE_SYSTEM_CONFIG_USE_SOCKETS && !LWIP_DNS

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -298,12 +292,11 @@ INET_ERROR DNSResolver::Cancel()
*/
void DNSResolver::HandleResolveComplete()
{
Weave::System::Object::AutoRelease objectRelease(*this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this as an anti-pattern. In this case we're dealing with a reference count, vs. a lock, where the lifetime of reference spans multiple states of the object. In this case, a single call to a method called Release() is clearly the most obvious pattern.


// 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();
}


Expand Down Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions src/system/SystemObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,21 @@ class NL_DLL_EXPORT Object
void Release(void);
Layer& SystemLayer(void) const;

class AutoRelease {
public:
AutoRelease(Object& owner) : mOwner(owner)
{
}

~AutoRelease(void)
{
mOwner.Release();
}

private:
Object& mOwner;
};

protected:
#if WEAVE_SYSTEM_CONFIG_USE_LWIP
/**< What to do when DeferredRelease fails to post a kEvent_ReleaseObj. */
Expand Down