New to openweave #492

marcosb · 2020-02-10T20:50:00Z

Hi,

Googler here looking to contribute; I was perusing the code and noticed there's a lot of manual locks/unlocks, instead of using RAII.

Is there a reason for not using SBRM here? It seems that if anything were to go wrong currently, the lock would be held indefinitely.

Thanks for your time, and if I missed something, let me know!

googlebot · 2020-02-10T20:50:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-02-10T20:50:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-02-10T22:31:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-02-10T22:31:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…eadlocks; see if this fixes it

codecov-io · 2020-02-11T19:26:22Z

Codecov Report

Merging #492 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
- Coverage   54.05%   54.04%   -0.01%     
==========================================
  Files         344      344              
  Lines       59061    59057       -4     
==========================================
- Hits        31924    31920       -4     
  Misses      27137    27137

Impacted Files	Coverage Δ
src/inet/AsyncDNSResolverSockets.cpp	`96.39% <100%> (-0.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9708816...fce2625. Read the comment docs.

jaylogue · 2020-02-12T02:36:04Z

Hi Marcos,

Thanks for your contribution. The pattern you propose is certainly not unacceptable for use in OpenWeave, especially given that the size and complexity of generated code is effectively the same. That said, I think we have steered away from this pattern because of its limited value in our context; specifically due to our preference for a single exit path for methods and the fact that we don't use exceptions. The other issue I see with RAII patterns is the poor readability for people not intimately familiar with the pattern--e.g. embedded C developers. The lack of definitive "lock is taken here" / "lock is released here" statements can obscure the true behavior of the code.

marcosb · 2020-02-12T18:08:50Z

Hey @jaylogue, thanks for the response! I probably picked a terrible example of where to use RAII, apologies for that - I picked a simple file just to get the discussion started.

Object::Release is probably a better example of multiple exit points & a place where RAII would be really useful; my thought was that if weave uses this pattern throughout, instances like this would not look so frightening & be so bug-prone:

https://github.com/openweave/openweave-core/blob/master/src/inet/DNSResolver.cpp

I can get a prototype CL for that file if you think it's worthwhile, just wanted to get a quick discussion going first.

marcosb · 2020-02-12T18:20:08Z

OK made the sample changes to DNSResolver; not totally sure if it compiles but I'll check once it's done.

At least it gives an idea though of how enforcing the RAII pattern (even in places where it doesn't help much) could help guide people to using this to write safer code.

E.g. if we go this route, we can actually make the Release method private, so that only AutoRelease can do the releasing, thus guiding engineers to use the safer & more readable auto-release method

marcosb · 2020-02-14T21:23:35Z

@jaylogue So this is interesting; you'll notice that in my changes to auto-release, one of the exit points wasn't releasing:
https://github.com/openweave/openweave-core/blob/master/src/inet/DNSResolver.cpp#L200

So there's 2 possibilities here:

This was a bug & it should've been releasing
This was WAI (I think it's reasonable to add a method to NOT auto-release in cases like this), but then there's no unit test coverage here

jaylogue · 2020-02-18T23:52:34Z

src/inet/DNSResolver.cpp

@@ -298,12 +292,11 @@ INET_ERROR DNSResolver::Cancel()
 */
 void DNSResolver::HandleResolveComplete()
 {
+    Weave::System::Object::AutoRelease objectRelease(*this);


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.

jaylogue · 2020-02-18T23:54:44Z

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);


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().

jaylogue · 2020-02-18T23:54:52Z

src/inet/AsyncDNSResolverSockets.cpp

@@ -172,8 +195,7 @@ INET_ERROR AsyncDNSResolverSockets::EnqueueRequest(DNSResolver &resolver)
 {
    INET_ERROR err = INET_NO_ERROR;
    int pthreadErr;
-
-    AsyncMutexLock();
+    LockHolder asyncMutexLock(*this);


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; }

jaylogue · 2020-02-18T23:54:57Z

src/inet/AsyncDNSResolverSockets.cpp

-
-    mInet->State = InetLayer::kState_ShutdownInProgress;
+    {
+        LockHolder asyncMutexLock(*this);


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); ... }

marcosb added 2 commits February 10, 2020 13:39

Remove manual locking/unlocking

de6ef5c

Use RAII for lock release

00ef599

Fix some errors post-compile

25827d5

marcosb added 5 commits February 11, 2020 11:11

By-reference passing is possibly changing pthread_mutex_t & causing d…

aa70426

…eadlocks; see if this fixes it

Missed param ref

9ed08b2

Use direct reference to class

000a781

Add LockHolder as friend for direct lock access

a3e7c4e

Update member field name

fce2625

marcosb added 2 commits February 12, 2020 11:15

AutoRelease for Object

397a55a

Use AutoRelease instead of manually calling Release()

2b1c818

marcosb added 3 commits February 12, 2020 11:44

Fix compilation

9d999fd

Stylistic fixes

d7fc131

Stylistic fixes

749856a

marcosb requested review from robszewczyk and jaylogue February 14, 2020 21:24

jaylogue reviewed Feb 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New to openweave #492

New to openweave #492

marcosb commented Feb 10, 2020

googlebot commented Feb 10, 2020

googlebot commented Feb 10, 2020

googlebot commented Feb 10, 2020

googlebot commented Feb 10, 2020

codecov-io commented Feb 11, 2020

jaylogue commented Feb 12, 2020

marcosb commented Feb 12, 2020

marcosb commented Feb 12, 2020

marcosb commented Feb 14, 2020

jaylogue Feb 18, 2020

jaylogue Feb 18, 2020

jaylogue Feb 18, 2020

jaylogue Feb 18, 2020

New to openweave #492

Are you sure you want to change the base?

New to openweave #492

Conversation

marcosb commented Feb 10, 2020

googlebot commented Feb 10, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Feb 10, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Feb 10, 2020

googlebot commented Feb 10, 2020

codecov-io commented Feb 11, 2020

Codecov Report

jaylogue commented Feb 12, 2020

marcosb commented Feb 12, 2020

marcosb commented Feb 12, 2020

marcosb commented Feb 14, 2020

jaylogue Feb 18, 2020

Choose a reason for hiding this comment

jaylogue Feb 18, 2020

Choose a reason for hiding this comment

jaylogue Feb 18, 2020

Choose a reason for hiding this comment

jaylogue Feb 18, 2020

Choose a reason for hiding this comment