NotificationsYou must be signed in to change notification settings
Fork219
Star781

Add argument to XPtr template to run finalizer on exit (fixes #655)#656

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

eddelbuettel merged 2 commits intoRcppCore:masterfromjeroen:xptr-onexit

Mar 16, 2017

Merged

Add argument to XPtr template to run finalizer on exit (fixes #655)#656

eddelbuettel merged 2 commits intoRcppCore:masterfromjeroen:xptr-onexit

Mar 16, 2017

Conversation

Copy link

Contributor

jeroen commentedMar 15, 2017

See#655. This adds an additional parameter toXPtr which specifies if the finalizer should run on exit in the same spirit as the underlyingR_RegisterCFinalizerEx.

I confirmed that this works. Default is stillFALSE so the current behavior will be unchanged.

Copy link

codecov-io commentedMar 15, 2017•
edited
Loading

Codecov Report

Merging#656 intomaster willnot change coverage.
The diff coverage is0%.

@@           Coverage Diff           @@##           master     #656   +/-   ##=======================================  Coverage   92.91%   92.91%           =======================================  Files          65       65             Lines        3303     3303           =======================================  Hits         3069     3069             Misses        234      234

Impacted Files	Coverage Δ
inst/include/Rcpp/XPtr.h	`62.16% <0%> (ø)`	✅

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update0566d7c...e3799de. Read thecomment docs.

add argument to XPtr template to run finalizer on exit

0ca897d

eddelbuettel approved these changes

Mar 15, 2017

View reviewed changes

Copy link

Member

eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks reasonable to me.

Copy link

ContributorAuthor

jeroen commentedMar 15, 2017

OK perfect, I'll add a NEWS item and bump the version.

Copy link

Member

eddelbuettel commentedMar 15, 2017

Great, and a changelog entry, please.

Are the data structures in your client packages C or C++? I am just wondering if you could also get this for free for C++ destructor semantics on your side . Just checking ...

nathan-russell approved these changes

Mar 15, 2017

View reviewed changes

jjallaire reviewed

Mar 15, 2017

View reviewed changes

inst/NEWS.Rd Outdated

		\itemize{
		\itemChangesinRcppAPI:
		\itemize{
		\itemXPtrgainsaparameter \code{finalizeoOnExit}toenablerunningthe

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Spelling of parameter here (finalizeoOnExit should be finalizeOnExit)

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

May also move the entry to the bottom. They are mostly chronological. In any even I can clean up both when finalizing the release "real soon now".

Copy link

Member

jjallaire commentedMar 15, 2017

@eddelbuettel I think the default finalizer will delete the C++ object thereby calling the destructor (so without this change there is actually no way to guarantee that the destructor is called prior to the process exiting)

@jeroen FWIW in some cases I've found it better to not let C++ destructors get called at process exit because the runtime context of an exiting process can be so stripped down (or non-deterministic across multiple C++ objects getting destroyed) that the destructor either crashes or hangs. i.e. sometimes it's better to just let the OS clean up than to try to explicitly do so within the process. Obviously this doesn't work in all cases, but I know that many things like mutexes, sockets, file handles, memory, etc. are freed automatically by the OS at process exit.

Copy link

Member

eddelbuettel commentedMar 15, 2017

Leaving the default atfalse is definitely the way to go.

Update NEWS, Changelog. Bump version.

e3799de

Copy link

ContributorAuthor

jeroen commentedMar 15, 2017

OK fixed the typo and moved the\item{} to the bottom.

Copy link

Member

eddelbuettel commentedMar 15, 2017

I launched a full reverse-depends check just in case, but we won't know til tomorrow how it does.

Copy link

Member

eddelbuettel commentedMar 16, 2017

Rev deps were all fine, results inRcppCore/rcpp-logs@8103eff

eddelbuettel merged commit6cf1301 intoRcppCore:master

Mar 16, 2017

jeroen deleted the xptr-onexit branch

March 16, 2017 16:11

Copy link

ContributorAuthor

jeroen commentedMar 16, 2017

Rev deps were all fine, results inRcppCore/rcpp-logs@8103eff

Let's send it to CRAN then :D

Copy link

Member

eddelbuettel commentedMar 16, 2017

Yes, which is why I ran the two previous reverse-depends. Probably by Friday.

Given the long tail of these reverse dependencies, Rcpp is a littler slower to make it from Incoming but you should be able to rely on it come next week.