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

Clean up (or remove) protect-unwind API and Rcpp_fast_eval #807

Closed
eddelbuettel opened this Issue Jan 27, 2018 · 19 comments

Comments

Projects
None yet
3 participants
@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 27, 2018

So #789 by @lionel- turned out to have had side effects. It is currently in hiding and #ifdef-ed away.

We could try to clean it up for the next release in March, or maybe the one thereafter in May (if we keep the same cadence). If we don't then we should take it out at some point. No rush, but should not get forgotten either.

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Jan 28, 2018

The RStudio team is going to be getting together in San Diego next week after rstudio::conf so hopefully @lionel- and I will have time to sit down and take a closer look.

The fact that the unwind stuff uniformly crashes on Windows is not promising, though (even with Rcpp out of the picture)

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Feb 3, 2018

Good news from Luke Tierney: he was able to reproduce the unwind crash I had seen on macOS / Linux and has a fix in R-devel for that.

If we reintroduce the usage of the unwind API we might still have to keep it turned off on Windows, though. (I think the crash may be toolchain related, but need to investigate a little more)

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Mar 14, 2018

@lionel- Rcpp 0.12.16 is out, so we would have some time to do damage if you want to revisit this now. Later is fine too.

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Mar 14, 2018

Thanks I'll revisit within a few weeks.

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Mar 25, 2018

Rcpp::internal::LongjumpException should not be internal to allow other packages to call potentially-jumping functions of the R API with protect-unwind. If the call fails, they should be allowed to throw a LongjumpException with their own unwind token.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Mar 25, 2018

Looks like it was you who put it there, so there are no other users yet. In that sense we could make that change.

But wasn't there some other verboten, don't do that state around longjumps? Or is my memory foggy?

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Mar 25, 2018

wasn't there some other verboten, don't do that state around longjumps

Are you referring to catching a LongjumpException? For protect-unwind it should be fine to continue a longjump with a token from someone else as long as the token was validly generated.

So LongjumpException shouldn't be caught by a third party without being rethrown (otherwise the longjump is interrupted and the token memory is leaked), but it's fine for a third party to throw their own LongjumpException. This also means LongjumpException has to be public so it can be rethrown before a catch (...).

Oh hmm maybe we need to think through what happens when another token is rethrown from a destructor (that could happen for instance if Rcpp_fast_eval() is called from a dtor). That would mean we leak the original token.

Edit: Ah we can release the token from the exception dtor. However it seems we can't guarantee proper propagation of an R longjump from a dtor because there might be another thrown exception on the stack. With C++11 it seems we might be able to get a hold on the longjump exception currently being thrown (with current_exception()) and update it with a new token. Still it seems that evaluating R code from a dtor should generally be avoided.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Mar 25, 2018

Sounds like something where we'd probably want to pass a spec or design to Luke for a glance at it.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented May 7, 2018

@lionel- Gentle poke. It is not urgent -- I'll probably prepare the next Rcpp release this week but if this is something we want to come back to for the next cycle let me know.

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented May 7, 2018

Thanks, I'm planning to work on this the week after eRum.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jun 9, 2018

Done in #859 -- with a big thank you!

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Jun 9, 2018

No worries! I hope all issues are now resolved... Let me know if anything comes up.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jun 9, 2018

We just got a new one -- both for me and for @coatless on macOS -- the 'embedded' code submitted by Filip. Not sure it is related, and don't mean to imply that, but it is something new. :-/

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Jun 9, 2018

Yes this is unrelated to the new eval stuff. You can solve this by sourcing in a child of the base env in https://github.com/RcppCore/Rcpp/blob/master/inst/unitTests/runit.embeddedR.R#L30.

newEnv <- new.env(parent = baseenv())
...
newEnv2 <- new.env(parent = baseenv())

However I am not sure why there's a dangling foo binding in the global env.

What's weird is that it doesn't always fail.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jun 9, 2018

Shall we open a new ticket for this? We all (ie Rcpp Core, @filipsch , ...) would probably benefit from what you know about environments and scoping...

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Jun 9, 2018

However I am not sure why there's a dangling foo binding in the global env.

Hmm it could also be in test.embeddedR() exec env or the RUnit sandbox for this file. Global env is a prime suspect because that's the default for sourceCpp().

Shall we open a new ticket for this? We all (ie Rcpp Core, @filipsch , ...) would probably benefit from what you know about environments and scoping...

Sure though I'm not sure when I'll have time to investigate this.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jun 9, 2018

That's ok, we are all busy as hell. @filipsch is the one who needs it and is most likely to drive this. If you can just help him by answering questions on ideas, code, design, approach,... it would help.

Else I may be forced to revert this as I am unlikely to release 0.12.18 with a known failure.

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Jun 9, 2018

By "this" you mean #852 right? It's unlikely to have introduced a new problem, the issue must be already there (perhaps in Rcpp but more likely in the unit tests).

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jun 9, 2018

Yes, what I turned off for now in this commit 5d5cdb8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.