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

Wrap pthread objects #9067

Merged
merged 7 commits into from Apr 30, 2020
Merged

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Apr 28, 2020

Short description

This PR wraps the remaining pthread_* objects in our code base:

  • pthread_mutex_t -> std::mutex (always correctly initialized, can't be copied, properly destroyed via RAII) ;
  • pthread_rwlock_t -> ReadWriteLock (light wrapper, always correctly initialized, can't be copied, properly destroyed via RAII) ;
  • pthread_create -> std::thread (no need of an awkward wrapper to pass the parameters, easier to join or detach).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@pieterlexis
Copy link
Contributor

pieterlexis commented Apr 28, 2020

Related: #6993

@rgacogne
Copy link
Member Author

rgacogne commented Apr 28, 2020

I missed a few spots, fixing that now.

@rgacogne
Copy link
Member Author

rgacogne commented Apr 28, 2020

Rebased to fix a conflict with #9069.

@rgacogne
Copy link
Member Author

rgacogne commented Apr 28, 2020

I missed a few spots, fixing that now.

Done, ready for review!

@omoerbeek
Copy link
Member

omoerbeek commented Apr 28, 2020

wrt lock.hh: Is g_singleThreaded still useful? AFAIK it is never set.
Is the class Lock still useful?

@rgacogne
Copy link
Member Author

rgacogne commented Apr 28, 2020

wrt lock.hh: Is g_singleThreaded still useful? AFAIK it is never set.

Yeah, I was looking at the same thing. I think it should go, the idea was nice but we never used it and IMHO this is an optimization best left to the implementation.

Is the class Lock still useful?

It's gone :)

@rgacogne
Copy link
Member Author

rgacogne commented Apr 28, 2020

I just removed g_singleThreaded.

@omoerbeek
Copy link
Member

omoerbeek commented Apr 29, 2020

I looked at making the constructors taking a pthread_rwlock_t * private to avoid re-introducing lock objects using pthread rw locks. That does compie, so if you agree I can push the diff to this branch.

@rgacogne
Copy link
Member Author

rgacogne commented Apr 29, 2020

I looked at making the constructors taking a pthread_rwlock_t * private to avoid re-introducing lock objects using pthread rw locks. That does compie, so if you agree I can push the diff to this branch.

Nice idea, I like it!

This is to avoid re-introducing code using the unwrapped pthread_rwlock_t's.
While there, reorganize the classes to make the order more natural.
Copy link
Member

@omoerbeek omoerbeek left a comment

Love how this makes all kinds of errors less likely to occur.

@rgacogne rgacogne merged commit 98e0a4b into PowerDNS:master Apr 30, 2020
29 checks passed
@rgacogne rgacogne deleted the wrap-pthread-objects branch Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants