r-quantities / units

Measurement units for R
https://r-quantities.github.io/units
175 stars 28 forks source link

Force units' finalizers before freeing the system #197

Closed Enchufa2 closed 5 years ago

Enchufa2 commented 5 years ago

This trick is not very elegant, but it's actually needed. The thing is, each time a unit is wrapped into an Rcpp XPtr, a weak finalizer is registered (which calls to ut_free eventually). This is a problem, because we can't control when they are executed. As a result, the package cannot be unloaded cleanly, because there's a segfault (builds succeeded because we hadn't warnings_are_errors set). The segfault happened because 1) ut_free_system was called upon unloading, and then 2) ut_free was called randomly in a weak finalizer. This call to gc() forces the execution of those pending finalizers before freeing the unit system.

And you'd be wondering why we didn't see this until now. The answer is that there was a typo here, so udunits_exit was not being called.

codecov[bot] commented 5 years ago

Codecov Report

Merging #197 into master will decrease coverage by 0.07%. The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   94.26%   94.18%   -0.08%     
==========================================
  Files          18       18              
  Lines         907      912       +5     
==========================================
+ Hits          855      859       +4     
- Misses         52       53       +1
Impacted Files Coverage Δ
R/init.R 16.66% <0%> (-1.52%) :arrow_down:
src/udunits.cpp 98.44% <100%> (+0.04%) :arrow_up:
R/user_conversion.R 81.81% <66.66%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7dc9be3...cea9bbf. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #197 into master will decrease coverage by 0.07%. The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   94.26%   94.18%   -0.08%     
==========================================
  Files          18       18              
  Lines         907      912       +5     
==========================================
+ Hits          855      859       +4     
- Misses         52       53       +1
Impacted Files Coverage Δ
R/init.R 16.66% <0%> (-1.52%) :arrow_down:
src/udunits.cpp 98.44% <100%> (+0.04%) :arrow_up:
R/user_conversion.R 81.81% <66.66%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7dc9be3...cea9bbf. Read the comment docs.

edzer commented 5 years ago

Looks good - thank!