duckdb vendors cpp11 0.4.2, which uses the "old" structure of the preserve list. It gets loaded first (and presumably something internally calls cpp11) so the preserve list is initialized to the "old" structure. Then we load arrow and that internally uses cpp11 0.4.4 which included that PR. Since the structures differ, the insertion/release calls seem to not be aligned quite right, so as R exits and the destructors for the arrow objects are all run, we end up with an error in SETCAR() which seems to come from the release() call in ~sexp().
The PR included an attempt to be backwards compatible with old versions of cpp11, but it seems like it wasn't quite right (and is quite hard to get right, as we see here).
We have had many issues with "shared cpp11 state" over the years, and the fact that a package built with, say, cpp11 0.4.2, has to share state with a package built with cpp11 0.4.4 makes it very hard to introduce changes to the shared state objects in a backwards compatible way.
In #327 we faced a related issue, and our solution was actually to remove that shared state altogether.
Lionel, Kevin, and I have come up with a proposal to do the same thing here. Rather than having 1 "global" preserve list that is shared across multiple package and across compilation units within the same package, we propose that we go back to the original implementation of the preserve list (before it was changed here https://github.com/r-lib/cpp11/pull/92), which creates one preserve list per compilation unit. The crux of the implementation would look like
static SEXP get_preserve_list() {
static SEXP out = R_NilValue;
if (out == R_NilValue) {
out = new_preserve_list();
R_PreserveObject(out);
}
return out;
}
We think this should get rid of the shared state problem altogether:
We no longer have to try and store a global preserve list somewhere. The static variable means that the pointer to it will persist across accesses, and R_PreserveObject() handles the protection for us. This means we no longer have to deal with storing external pointers as global options (which is the only thing we could use due to needing to avoid the option being copyable and needing to avoid a stack overflow when serializing the protection list). It also means a user can't bork it accidentally.
There would be minimal memory increase. We estimate it at sizeof(empty preserve list) * number of compilation units, which seems reasonable.
It fixes the duckdb / arrow issue because duckdb can continue using their vendored version (for now) and it won't conflict with arrow if it gets built with this new cpp11 version since each arrow compilation unit has its own preserve list.
I tested the reprex in this issue https://github.com/tidyverse/tidyr/issues/1059 which was supposedly fixed by switching to a global preserve list, but it does not seem to reproduce in my tests when switching back to a one-list-per-compilation-unit approach.
Two very minor downsides:
release_all() is a helper that was intended for R < 3.5.0 that would release all cpp11 objects from the preserve list. This would no longer work as expected since it would only release objects from the list managed by that compilation unit. But we think that is okay because we don't think this function is particularly safe anyways - you could release an object that you don't own if the preserve list is a global one! A github search makes us think no one is using it, and the tidyverse / r-lib orgs have moved on to require R >= 3.6.0 at this point, so I think we can set R (>= 3.5.0) for cpp11 and remove this function safely. It seems like release_all() was one of the motivating reasons for a global preserve list according to #92, so if we remove that then that is even more reason to consider alternative approaches.
If you are developing, say, vroom, and have some vroom R objects (which may be ALTREP and may rely on objects in the preserve list existing) lying around and rerun load_all(), then that will reload the C++ internals resulting in new preserve lists being created. We think this is okay, as the old preserve lists will have been protected with R_PreserveObject() so the R vroom objects should continue to work, but their memory will not be released until R restarts. This seems okay since it would be a very rare developer only issue.
The following will error if arrow is compiled with cpp11 0.4.4
It should error on restart with:
This error is related to
SETCAR()calls, which we confirmed with an lldb backtrace:That error is due to the changes in this PR https://github.com/r-lib/cpp11/pull/285, which slightly tweaked the structure of the preserve list.
duckdb vendors cpp11 0.4.2, which uses the "old" structure of the preserve list. It gets loaded first (and presumably something internally calls cpp11) so the preserve list is initialized to the "old" structure. Then we load arrow and that internally uses cpp11 0.4.4 which included that PR. Since the structures differ, the insertion/release calls seem to not be aligned quite right, so as R exits and the destructors for the arrow objects are all run, we end up with an error in
SETCAR()which seems to come from therelease()call in~sexp().The PR included an attempt to be backwards compatible with old versions of cpp11, but it seems like it wasn't quite right (and is quite hard to get right, as we see here).
We have had many issues with "shared cpp11 state" over the years, and the fact that a package built with, say, cpp11 0.4.2, has to share state with a package built with cpp11 0.4.4 makes it very hard to introduce changes to the shared state objects in a backwards compatible way.
In #327 we faced a related issue, and our solution was actually to remove that shared state altogether.
Lionel, Kevin, and I have come up with a proposal to do the same thing here. Rather than having 1 "global" preserve list that is shared across multiple package and across compilation units within the same package, we propose that we go back to the original implementation of the preserve list (before it was changed here https://github.com/r-lib/cpp11/pull/92), which creates one preserve list per compilation unit. The crux of the implementation would look like
We think this should get rid of the shared state problem altogether:
staticvariable means that the pointer to it will persist across accesses, andR_PreserveObject()handles the protection for us. This means we no longer have to deal with storing external pointers as global options (which is the only thing we could use due to needing to avoid the option being copyable and needing to avoid a stack overflow when serializing the protection list). It also means a user can't bork it accidentally.sizeof(empty preserve list) * number of compilation units, which seems reasonable.Two very minor downsides:
release_all()is a helper that was intended for R < 3.5.0 that would release all cpp11 objects from the preserve list. This would no longer work as expected since it would only release objects from the list managed by that compilation unit. But we think that is okay because we don't think this function is particularly safe anyways - you could release an object that you don't own if the preserve list is a global one! A github search makes us think no one is using it, and the tidyverse / r-lib orgs have moved on to require R >= 3.6.0 at this point, so I think we can setR (>= 3.5.0)for cpp11 and remove this function safely. It seems likerelease_all()was one of the motivating reasons for a global preserve list according to #92, so if we remove that then that is even more reason to consider alternative approaches.load_all(), then that will reload the C++ internals resulting in new preserve lists being created. We think this is okay, as the old preserve lists will have been protected withR_PreserveObject()so the R vroom objects should continue to work, but their memory will not be released until R restarts. This seems okay since it would be a very rare developer only issue.Some other related issues: