Make Rcpp::exception thread-safe #1043

jpritikin · 2020-02-09T16:07:36Z

Pull Request Template for Rcpp

See the discussion thread on rcpp-devel.

I don't think new tests are a great idea. It would be tricky to reliably fail by throwing the old thread-unsafe version of Rcpp::exception.

Checklist

Code compiles correctly
R CMD check still passes all tests
Prefereably, new tests were added which fail without the change
Document the changes by file in ChangeLog

eddelbuettel · 2020-02-09T16:10:12Z

Well this falls short on

issue ticket with prior discussion in order to establish consensus about what to change, rather than throwing a PR over the fence
does not demonstrate lack of impact on reverse dependencies, and it is not fair of you to assume I have to do it for you

eddelbuettel · 2020-02-09T16:16:33Z

Also, I did point out to you on rcpp-devel that we had extensive discussions around that. Maybe take a moment and look at

Improvements for unwind-protect #877 with 12 comments
Extract Rcpp::unwindProtect() from Rcpp::Rcpp_fast_eval() #873 with 24 comments
Control whether exceptions include the call and C++ stack with a macros #868 with 10 comments
Switching from Rcpp_eval to Rcpp_fast_eval. close #866 #867 with 44 comments
Fix protect-unwind #859 with 31 comments
Use protect-unwind API and add Rcpp_fast_eval() #789 with 60 comments
Protect function argument to rcpp_set_stack_trace() #706 with 14 comments

which may be relevant. There is much more via a simple search for 'exception' among closed PRs.

jpritikin · 2020-02-09T16:34:42Z

issue ticket with prior discussion in order to establish consensus about what to change, rather than throwing a PR over the fence

I'm happy to open an issue, but why not just have the discussion here?

Also, I did point out to you on rcpp-devel that we had extensive discussions around that.

Hm, I didn't see these because I searched issues instead of PRs. The only one that looks relevant is #868. It seems like this PR was incomplete because rcpp_set_stack_trace was not conditioned on include_call. So I am happy to add another commit to obey include_call if you think that is the right thing to do.

Basically, my approach here was to make Rcpp::exception thread-safe and otherwise change as little as possible. So what's the next step?

jpritikin · 2020-02-09T16:36:09Z

does not demonstrate lack of impact on reverse dependencies, and it is not fair of you to assume I have to do it for you

What's an efficient way to demonstrate a lack of impact on reverse dependencies? There are a zillion packages that depend on Rcpp.

kevinushey · 2020-02-09T17:02:32Z

This requires a reverse depends check, which does imply running R CMD check on all client packages of Rcpp. @eddelbuettel usually runs these after PRs which may be disruptive. (There's also the revdepcheck package for doing these kinds of checks as well)

eddelbuettel · 2020-02-09T17:06:21Z

I wrote a package for that (see prrd) which I use on a somewhat underpowered VM I have a courtesy account on, but that box is busy today rev.dep checking RcppArmadillo which I submitted. That takes ~ 6 hours, Doing Rcpp takes 18+ hours. And I have some work to do today so I cannot dedicated my Sunday for doing the work for PRs we have not deemed necessary yet, and which go fairly deep into inner Rcpp guts where we needed half-a-dozen PRs to get to where we are today.

Also, how to do reverse dependency checks is otherwise not a secret, discussed on the R mailing lists, supported by base R and several add-on packages.

eddelbuettel · 2020-02-09T17:08:06Z

There is no impact on reverse dependencies. I merely reorganized the existing code.

"Beware of bugs in the above code; I have only proved it correct, not tried it.". -- Donald Knuth

jpritikin · 2020-02-09T17:46:30Z

This requires a reverse depends check, which does imply running R CMD check on all client packages of Rcpp.

Wow. Okay, I kicked off the job.

kevinushey

Mostly LGTM, but I would prefer to keep the old stack_trace routine registered. (It's okay if it's just a stub that returns R_NilValue)

inst/include/Rcpp/exceptions.h

inst/include/Rcpp/routines.h

inst/include/Rcpp/exceptions_impl.h

inst/NEWS.Rd

inst/include/Rcpp/exceptions.h

eddelbuettel · 2020-02-09T18:58:09Z

@lionel- I couldn't add you to 'request a code review' but it would be good if you could look at this too. If you have a few moments in the next few days it would be appreciated!

jpritikin · 2020-02-09T21:33:52Z

Also, how to do reverse dependency checks is otherwise not a secret, discussed on the R mailing lists, supported by base R and several add-on packages.

If you are so tired of answering, you might add some hints in the Contributing.md file.

eddelbuettel · 2020-02-09T22:06:39Z

The file, like everything else here, is based on volunteer effort and time, and in fact fairly recent. It appears to have helped as well because your PR, while clearly not ready yet or consensus-based, did in fact 'tick off' a couple of boxes I consider helpful.

If you have a suggestion to make for the file, why not raise an issue ticket and suggest new wording?

Lastly, I am sorry but as I recall you too have multiple CRAN uploads under your belt and state each time you do one that you checked reverse depends (if there any) for your package. In other words you were already supposed to know this, which is why I allowed myself to use short hand.

jpritikin · 2020-02-09T22:13:00Z

Lastly, I am sorry but as I recall you too have multiple CRAN uploads under your belt and state each time you do one that you checked reverse depends (if there any) for your package. In other words you were already supposed to know this, which is why I allowed myself to use short hand.

I do indeed have a primitive script to check reverse depends, but nowhere near as polished as the scripts you suggested. It is one thing to check when there are less than 10 revdeps, and other thing entirely when there are more than 2000. Cheers!

eddelbuettel · 2020-02-09T22:18:22Z

1857 as of today. We omit Suggests:.

And I am aware that this is a lot of work. I have been doing this diligently for some time now: https://github.com/RcppCore/rcpp-logs

It is to the benefit of everybody who depends on Rcpp, i.e. all of use here and of course you too. And if "stuff breaks" I have to deal with it which is why I lean towards the conservative side when it comes to unannounced PRs going deep into internals.

And as a whole we could do better around CRAN and tooling but alas there is always so much to do and so little time. Again, if you think you can make the process better please do not hold back (and I of course do not speak for CRAN as nobody does).

lionel- · 2020-02-10T10:10:37Z

LGTM!

jpritikin · 2020-02-10T12:58:18Z

I tried to run revdepcheck overnight and my laptop ran out of memory. Sorry but it seems that I don't have access to big enough hardware to run the check. I would appreciate it if somebody else can do it. No hurry; This can happen at your convenience.

eddelbuettel · 2020-02-10T13:01:12Z

Now that RcppArmadillo is updated on CRAN, I can turn this on. Runs unattended in a (very remote to me) data center anyway once I launch. But what about comments / requests for changes Kevin made yesterday? (I didn't have time to look in detail, was busy shepherding RcppArmadillo and cranking out a paper draft...)

jpritikin · 2020-02-10T14:08:28Z

But what about comments / requests for changes Kevin made yesterday?

I believe that I have addressed them.

inst/include/Rcpp/exceptions_impl.h

eddelbuettel · 2020-02-10T14:30:37Z

And just to be sure: you squashed/rebased your several commits back into one? It is a little hard to see what changed when in that case. Possibly nicer for merged history yet harder for PR assessment...

jpritikin · 2020-02-10T14:36:50Z

And just to be sure: you squashed/rebased your several commits back into one? It is a little hard to see what changed when in that case. Possibly nicer for merged history yet harder for PR assessment...

Yeah, I wasn't sure which kind of history was preferred. That's another point to add to the Contributing.md file.

eddelbuettel · 2020-02-10T14:37:26Z

Well I guess all we know now is that I just wasted the last few minutes preparing a test tarball.

eddelbuettel · 2020-02-10T14:40:48Z

And I am in no position to assess what just changed. That goes against the entire spirit of the PR we all just spent 24 hours on. Why?

jpritikin · 2020-02-10T14:46:11Z

And I am in no position to assess what just changed. That goes against the entire spirit of the PR we all just spent 24 hours on. Why?

I changed the authorship in the comment. That's it. From now on, I'll add commits instead of squashing.

jpritikin · 2020-02-10T14:47:16Z

If you want, I can unsquash the commits. But that would require another force push. Let me know.

eddelbuettel · 2020-02-10T14:48:42Z

I would appreciate that as it will no longer hide your work and make it inaccessible. It is also how the rest the world works. If and when we're done you can still squash, or I could squash-merge, ...

+ Move NEWS.Rd entries to the bottom of the relevant section + Restore GET_STACKTRACE macro + Move open brace to conform to code style + Add copywrite comment block to Rcpp/exceptions_impl.h + Restore useless stack_trace API in Rcpp/routines.h + Restore RCPP_REGISTER(stack_trace)

eddelbuettel · 2020-02-10T15:07:35Z

Jeebus.

Did you just push AGAIN?

eddelbuettel · 2020-02-10T15:08:29Z

The 👎 was a discouragement. As in: "no, I would prefer if you not pushed again".

jpritikin · 2020-02-10T15:10:03Z

I would appreciate that as it will no longer hide your work and make it inaccessible. It is also how the rest the world works. If and when we're done you can still squash, or I could squash-merge, ...

The -1 was a discouragement. As I, "no, I would prefer if you not pushed again".

What the heck? You are giving contradictory instructions within the span of 30 minutes. Yeah, I pushed again to unsquash the commits.

Sorry, I didn't notice the thumbs down emoji until it was too late.

eddelbuettel · 2020-02-10T15:10:29Z

I'll unsubscribe from this thread now. I actually have work to do.

eddelbuettel · 2020-02-12T02:17:20Z

@kevinushey When you have a moment, can you take a look at the changes made in response to you code review comments? I have "unresolved" these; commonly the person who raises an issue gets to declare it resolved (rather than the person responding, but many things were a little unusual here...)

I had started one full reverse-depends which was promptly partially invalidated by forced pushes, but maybe that won't happen again either. In any, results from the rev.dep runs are in RcppCore/rcpp-logs@dc22889 and RcppCore/rcpp-logs@6811463. Result are pretty stunning -- essentially only three fails on the last run all of which seem internal to the affected packages (but as usual I do skip a few for various reasons).

So in sum no issue from here: looks good. Shout-out also to @lionel- for taking a look, appreciate it,

kevinushey

LGTM; just two minor nitpicks.

kevinushey · 2020-02-12T04:51:30Z

ChangeLog

+	* inst/include/Rcpp/exceptions.h: Make thread-safe
+	* inst/include/Rcpp/exceptions_impl.h: New home for code moved
+	from inst/include/Rcpp/routines.h and src/api.cpp
+	* src/rcpp_init.cpp: stack_trace no longer exported as an R API


nit: this is no longer true I think?

kevinushey · 2020-02-12T04:52:36Z

inst/include/Rcpp/exceptions_impl.h

@@ -0,0 +1,88 @@
+// exceptions_impl.h: Rcpp R/C++ interface class library -- exceptions
+//
+// Copyright (C) 2020 Joshua N. Pritikin


We should preserve the old author names as well (adding yourself is okay since you've also made changes)

eddelbuettel · 2020-02-12T05:00:36Z

I can take care of the cleanup on the header and ChangeLog and NEWS entries.

jpritikin · 2020-02-12T12:42:27Z

Thanks all! Sorry again for the force pushes.

eddelbuettel · 2020-02-12T12:44:26Z

It's ok, I think we're all coming closer to the same page regarding process now.

Do you want to do another commit with touch-ups? Or do you want me to?

Make Rcpp::exception thread-safe

2815ae8

jpritikin requested a review from eddelbuettel February 9, 2020 16:21

kevinushey reviewed Feb 9, 2020

View reviewed changes

eddelbuettel reviewed Feb 9, 2020

View reviewed changes

inst/NEWS.Rd Outdated Show resolved Hide resolved

inst/include/Rcpp/exceptions.h Outdated Show resolved Hide resolved

jpritikin force-pushed the master branch from 2815ae8 to 28dd2e3 Compare February 9, 2020 21:05

eddelbuettel reviewed Feb 10, 2020

View reviewed changes

inst/include/Rcpp/exceptions_impl.h Outdated Show resolved Hide resolved

jpritikin force-pushed the master branch from 28dd2e3 to cdb3087 Compare February 10, 2020 14:34

jpritikin added 2 commits February 10, 2020 09:52

Change copyright authorship per at Dirk's direction

614e72f

jpritikin force-pushed the master branch from cdb3087 to 614e72f Compare February 10, 2020 14:57

kevinushey approved these changes Feb 12, 2020

View reviewed changes

jpritikin added 2 commits February 12, 2020 08:58

Append to instead of replace authorship

23709a2

Update ChangeLog per latest PR revision

caa35fb

eddelbuettel approved these changes Feb 12, 2020

View reviewed changes

eddelbuettel merged commit 30fdafc into RcppCore:master Feb 12, 2020

eddelbuettel added a commit that referenced this pull request Feb 12, 2020

minor post-processing for #1043

c9065d5

eddelbuettel mentioned this pull request Mar 23, 2020

bad_alloc crash on throw with -D_GLIBCXX_USE_CXX11_ABI=0 flag #1058

Closed

Make Rcpp::exception thread-safe #1043

Make Rcpp::exception thread-safe #1043

Conversation

jpritikin commented Feb 9, 2020

Pull Request Template for Rcpp

Checklist

eddelbuettel commented Feb 9, 2020

eddelbuettel commented Feb 9, 2020 • edited Loading

jpritikin commented Feb 9, 2020 • edited Loading

jpritikin commented Feb 9, 2020 • edited Loading

kevinushey commented Feb 9, 2020

eddelbuettel commented Feb 9, 2020 • edited Loading

eddelbuettel commented Feb 9, 2020

jpritikin commented Feb 9, 2020

kevinushey left a comment

Choose a reason for hiding this comment

eddelbuettel commented Feb 9, 2020

jpritikin commented Feb 9, 2020 • edited Loading

eddelbuettel commented Feb 9, 2020

jpritikin commented Feb 9, 2020

eddelbuettel commented Feb 9, 2020

lionel- commented Feb 10, 2020

jpritikin commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020

jpritikin commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020

jpritikin commented Feb 10, 2020 • edited Loading

eddelbuettel commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020

jpritikin commented Feb 10, 2020

jpritikin commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020

eddelbuettel commented Feb 10, 2020 • edited Loading

jpritikin commented Feb 10, 2020 • edited Loading

eddelbuettel commented Feb 10, 2020

eddelbuettel commented Feb 12, 2020

kevinushey left a comment

Choose a reason for hiding this comment

kevinushey Feb 12, 2020

Choose a reason for hiding this comment

kevinushey Feb 12, 2020

Choose a reason for hiding this comment

eddelbuettel commented Feb 12, 2020

jpritikin commented Feb 12, 2020

eddelbuettel commented Feb 12, 2020

eddelbuettel commented Feb 9, 2020 •

edited

Loading

jpritikin commented Feb 9, 2020 •

edited

Loading

jpritikin commented Feb 9, 2020 •

edited

Loading

eddelbuettel commented Feb 9, 2020 •

edited

Loading

jpritikin commented Feb 9, 2020 •

edited

Loading

jpritikin commented Feb 10, 2020 •

edited

Loading

eddelbuettel commented Feb 10, 2020 •

edited

Loading

jpritikin commented Feb 10, 2020 •

edited

Loading