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

Fix memory leak with move assignment operator #366

Merged

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Jul 28, 2024

Closes #338

This is rather complicated, and is caught up in the fact that we currently double protect all writable r_vectors (oops), but this is the minimal fix for #338. More changes are coming to simplify things further.

Here's the basic idea of what was happening (glossing over double protect weirdness). Take this example:

cpp11::writable::integers x(1);
x = cpp11::writable::integers(1);

Here are the protection counts:

  • +1 x The constructor creates x and protects it
  • +1 <temp> An rvalue (temporary) of cpp11::writable::integers(1) is created and protected
  • +1 <temp> WRONG! The move assignment operator is re-protecting the rvalue SEXP as it takes ownership of it rather than just taking the protect token from the rhs.
  • -1 x The move assignment operator releases old x
  • The move assignment operator sets rhs.protect_ = R_NilValue so when rhs is destructed, it can't release anything

So if you add that up you end with a +2 count of items protected in the protection list when it should be +1, and that was indeed what I was seeing in my new tests. The memory of the <temp> value can never be released because the protection list always holds a reference to it.

The solution is simple - we just move the rhs.protect_ value into x rather than generating our own. This was probably the intention the whole time anyways, since we were also doing rhs.protect_ = R_NilValue at the end.

@DavisVaughan DavisVaughan force-pushed the fix/rvalue-reference-assignment-leak branch 2 times, most recently from 276059b to ee55022 Compare July 28, 2024 14:47
@DavisVaughan DavisVaughan force-pushed the fix/rvalue-reference-assignment-leak branch from ee55022 to 057de9e Compare July 28, 2024 17:23
@DavisVaughan DavisVaughan merged commit 86a6a3e into r-lib:main Jul 28, 2024
16 checks passed
@DavisVaughan DavisVaughan deleted the fix/rvalue-reference-assignment-leak branch July 28, 2024 17:34
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

Successfully merging this pull request may close these issues.

Memory leak with reassigning cpp11::writable::list, and maybe others
1 participant