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

make our Lock classes uncopyable, make ReadLock and WriteLock moveable. #5209

Merged
merged 4 commits into from Apr 11, 2017

Conversation

Projects
None yet
2 participants
@ahupowerdns
Member

ahupowerdns commented Mar 28, 2017

Short description

Right now it is possible to copy our Lock classes, after which bad things will happen. Both destructors will attempt to unlock the mutex.

With this PR 1) copying becomes impossible 2) the ReadLock and WriteLock classes gain move support, which enables storing them in containers with expected semantics. This is good for having "thousands of locks" that exhibit RAII behaviour.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master
@rgacogne

Looks good, but ReadLock::upgrade() should probably check d_lock and throw an exception if it's equal to nullptr.

@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne Mar 28, 2017

Member

Alternatively, perhaps ReadLock::upgrade() should just go since it doesn't appear to be used and is quite misleading, because it does release the existing read lock before acquiring a write one.

Member

rgacogne commented Mar 28, 2017

Alternatively, perhaps ReadLock::upgrade() should just go since it doesn't appear to be used and is quite misleading, because it does release the existing read lock before acquiring a write one.

@ahupowerdns

This comment has been minimized.

Show comment
Hide comment
@ahupowerdns

ahupowerdns Apr 7, 2017

Member

I have added some tests and removed the upgrade() feature which was stupid. gotIt() will give you a nonsense answer on moved locks, but I'm told you should not be doing that.

Member

ahupowerdns commented Apr 7, 2017

I have added some tests and removed the upgrade() feature which was stupid. gotIt() will give you a nonsense answer on moved locks, but I'm told you should not be doing that.

@ahupowerdns ahupowerdns merged commit 9fffec2 into PowerDNS:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ahupowerdns ahupowerdns deleted the ahupowerdns:lock-touches branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment