Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove recusive memberlist locking #6662

Merged
merged 10 commits into from Jun 20, 2019

Conversation

@baumanj
Copy link
Member

commented Jun 14, 2019

Since RwLocks can't be both fair and recursive (and recursive locking is very hard to reason about), let's eliminate the recursive locking of the locks in MemberList. See the individual commit messages for more explanation.

See #6435

@baumanj baumanj added the X-fix label Jun 14, 2019

@baumanj baumanj self-assigned this Jun 14, 2019

@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 14, 2019

Hello baumanj! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

baumanj added some commits Jun 11, 2019

Annotate MemberList methods' locking requirements
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Add health_of_with_memberships test
This will replace the health_of test, which requires recursive locking

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Annotate locking dependencies and fix up thread spawning
Add both comments and name suffixes to indicate what locks are taken:
mlw:  MemberList::entries (write)
mlr:  MemberList::entries (read)
imlw: MemberList::intitial_entries (write)
imlr: MemberList::intitial_entries (read)

This may be removed later, but in the short term should help track down
instances of recursive locking we wish to eliminate.

Relatedly, change the way we spawn various threads in the supervisor to
handle spawning errors and make the locking implications clearer.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Fix clippy lints
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Futher locking annotation and cleanups
Make some with_closure arguments bound to Fn rather than FnMut

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Eliminate recursive MemberList locking in outbound::probe_mlr
In order to avoid the need to access the member list inside the
with_pingreq_targets_mlr, move the call to
populate_membership_rumors_mlr earlier. The most obvious potential risk
is if there is a significant write to the member list between the call
populate_membership_rumors_mlr and with_pingreq_targets_mlr, the
PingReq message could contain stale data, but that doesn't seem very
likely or of great concern.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Eliminate recursive MemberList locking in inbound::process_pingreq_mlr
Instead calling outbound::ping from within the with_member_mlr closure,
use get_cloned_mlr to duplicate the target member.

Also, convert populate_membership_rumors_mlr to consume the template
message and return a Swim struct rather that operating on a &mut Swim.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Eliminate recursive MemberList locking in populate_census
Switch from with_members_mlr to with_memberships_mlr to eliminate the
need to call health_of_mlr inside the closure.

Also, remove with_members_mlr and with_member_mlr now that they are no
longer used.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Fix bare_trait_objects warning
Another approach would've been to add `dyn` to the type of `r`.
Instead, this changes (potential) dynamic dispatch to a monomorphized
generic type for static dispatch.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>

@baumanj baumanj force-pushed the remove-recusive-memberlist-locking branch from 2638c77 to ca03cc4 Jun 19, 2019

@baumanj baumanj marked this pull request as ready for review Jun 20, 2019

Increase windows studio CI test timeout
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
strukt.end()
}
}

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 20, 2019

Member

Why was this removed?

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 20, 2019

Author Member

It didn't appear that it was used. Since Server doesn't derive Serialize and removing this impl Serialize for Server didn't cause any complication errors, I concluded all serialization needs were being handled by ServerProxy. Removing impl<'a> Serialize for ServerProxy<'a> does break the build. Did I miss something?

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 20, 2019

Member

Ah, that's right. I forgot about ServerProxy.

@raskchanky
Copy link
Member

left a comment

I'm curious why the Serialize implementation for the butterfly server was removed, because that will break the HTTP gateway, but other than that, this looks great.

@baumanj baumanj merged commit a3380d9 into master Jun 20, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2435 passed (30 minutes, 27 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2878 passed (3 minutes, 35 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@baumanj baumanj deleted the remove-recusive-memberlist-locking branch Jun 20, 2019

chef-ci added a commit that referenced this pull request Jun 20, 2019

Update CHANGELOG.md with details from pull request #6662
Obvious fix; these changes are the result of automation not creative thinking.

baumanj added a commit that referenced this pull request Jun 25, 2019

Add locking annotations that were overlooked
These should've been included in #6662

Fortunately, no additional logic changes are required.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>

baumanj added a commit that referenced this pull request Jun 25, 2019

Add locking annotations that were overlooked
These should've been included in #6662

Fortunately, no additional logic changes are required.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.