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

Make Borrowable's descructor warn / crash if destructed with outstanding borrows #47

Open
CodingCanuck opened this issue May 25, 2022 · 0 comments

Comments

@CodingCanuck
Copy link
Contributor

Borrowable's destructor currently hangs until all borrows of the object have been relinquished:

// NOTE: currently this implementation of Borrowable does an atomic
// backoff instead of blocking the thread when the destructor waits
// for all borrows to be relinquished. This will be much less
// efficient (and hold up a CPU) if the borrowers take a while to
// relinquish. However, since Borrowable will mostly be used in
// cirumstances where the tally is definitely back to 0 when we wait
// no backoff will occur. For circumstances where Borrowable is being
// used to wait until work is completed consider using a Notification
// to be notified when the work is complete and then Borrowable should
// destruct without any atomic backoff (because any workers/threads
// will have relinquished).

I'd call this surprising behavior: it's not mentioned in documentation that I can see (e.g. not in the old borrowed_ptr README), and it's sharply different behavior from other standard c++ smart pointers (namely std::unique_ptr, std::shared_ptr, and std::weak_ptr) and boost smart pointers.

Let's print a warning warns if a borrrowable destructor is called without outstanding non-relinquished borrows. A warning would have saved me several days of debugging: I had written buggy code where a borrowable was destructed before its uses were relinquished. I didn't expect incorrect Borrowable use to cause deadlocks, and I wasn't able to diagnose the issue without attaching a debugger.

Better yet, let's make this default behavior and remove the default of atomic backoff. (Atomic backoff feels like an orthogonal feature to a safe alternative to raw pointers: if it's something needed by some use cases, perhaps there could be a WaitingBorrowedPointer or similar that adds this extra backoff functionality in an opt-in fashion that most callers would only use if they have shared / non-simple ownership).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant