Reset errors after evaluation (closes #312)

[kevinushey]

This fixes a bug where nested calls to Rcpp functions (through Rcpp_eval) would end up seeing the same error stack, and hence all would report the same error.

[jjallaire]

Looks good to me. @eddelbuettel ?

[eddelbuettel]

Bit fried and tired after two days of Rcpp teaching here in Zuerich ...

We can keep it in a branch, or merge it as is, or maybe tests along the to-be-tested String change. I should have some time for all of this over the next few days. Or so I hope ...

But feel free to take the lead on this and don't wait on me if you feel it's ready.

[jcheng5]

Thanks for jumping on this @kevinushey.

This case fails, I'd expect it to be silent but I get an error:

exec(function() {
  try(silent = T, exec(function() {
    options(warn = 2)
    warning("hi")
  }))
})

[kevinushey]

Thanks @jcheng5. This is another manifestation of the error issue before: the warning "hi" gets reported twice, once for each Rcpp_eval call. I'll update the PR to ensure this doesn't happen.

[kevinushey]

I have a candidate fix for the warning bit -- ensure that withCallingHandlers is only attached to top-level calls. However, the implementation right now is really dirty and I'm not really sure if this is the best way of going about it.

Similarly, I wonder if both tryCatch and withCallingHandlers should only be attached at the top level (do we want to wrap tryCatch around each individual evaluation?)

[kevinushey]

I think we are basically forced to attach calling handlers only at the top level. This is what the warning recorders see in that call (minus the most recent commit on this PR):

> exec(function() {
+     try(silent = TRUE, exec(function() {
+         warning("ouch")
+     }))
+ })
$message
[1] "ouch"

$call
(function () 
{
    warning("ouch")
})()

$message
[1] "ouch"

$call
exec(function() {
    warning("ouch")
})

So, both recorders see the same warning message, and so both attempt to forward the same warning. The error is then seen when the warning is forwarded outside of the try call.

EDIT: I think I may understand what's happening. When we attempt to print out captured warnings, we don't evaluate it in a top-level context -- ie, call Rf_warning within the current evaluation context. Since a higher-level handler (attached to Rcpp_eval) is already listening for warnings, it actually grabs that warning, and then later emits that warning again through Rf_warning.

In other words, the warning gets forwarded by each handler from the deepest level of execution all the way to the top level, where it is then emitted (potentially bypassing other calling handlers that may influence how the warning should have been printed).

I do not yet know what the best way of handling this is -- I think each evaluation effectively needs to emit its warnings, but outside the context of the Rcpp_eval-attached handlers.

That said -- I still do not have a handle on what the best fix here is. It seems like we do want handlers attached to each Rcpp_eval call, but we want to tell parent handlers to ignore warnings emitted by sub-handlers... somehow. I don't think there's a way to 'attach' information to a particular warning as it gets handled by each registered handler.

[jjallaire]

this should be initialized to zero I think

[jjallaire]

@kevinushey this looks ready for merge to me. Any objection to doing so now?

[eddelbuettel]

I'd be happy to do so and try to turn on the rev dep run a little later.

[jjallaire]

Actually, I think there might be one more subtlety to address. Since the static depth counter is within an inline function I'm thinking that each package which uses Rcpp will get it's own depth counter (I could be wrong though) and we still may end up with the same issue when multiple packages which include Rcpp try to play together (likely in the case of httpuv as it's callbacks often invoke arbitrary other code in packages used within shiny applications).

I'm I'm right then we need a static depth counter which is incremented and decremented using functions exposed from libRcpp.so (sort of like we do with the RNG scope counter now).

@kevinushey What do you think?

[kevinushey]

@jjallaire I think you're exactly right.

I think this PR should be split into two pieces:

1. The change to ensure errors are reset after evaluation (ie, the first part of this PR),
2. Better handle warnings + messages (as per @jcheng5's comment)

I am pretty confident that (1) is the correct change there; (2) is going to be quite tricky to get right. So I'll strip out the latest commit and then I think we can merge that (so that errors are properly handled); we can then handle (2) in a separate PR.

[kevinushey]

I'm going to merge this now; we can deal with the other issues in a separate PR.

[jjallaire]

Sounds good. Dirk and I had a discussion regarding whether we should make
this next change one that "breaks" users running with new headers against
old versions of libR.so (a configuration which is not only possible but
likely given the way package updates/dependencies work).

It's obviously cleaner to insist on the integrity of version-synced headers
<-> shared library however it will cause of bunch of (temporary) breakage
in the field if we don't code around it. The way to code around it would be
to look for the static depth counter increment/decrement functions in the
shared library but NOT fail (but rather revert to the old non-depth counted
behavior) if they aren't there.

Let's settle this over email / PR over the next few days.

On Sun, Jun 28, 2015 at 5:10 PM, Kevin Ushey notifications@github.com
wrote:

Merged #313 #313.

—
Reply to this email directly or view it on GitHub
#313 (comment).

[jjallaire]

What I'm thinking we would do is to modify this code for calling the
exported C function (normally generated via Rcpp::register):

https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/routines.h#L77-L81

To check for whether it actually got a function pointer or not. If it
didn't get one then it essentially no-ops back to the previous behavior
(depth == 0).

I realize that this isn't ideal, but breaking everyone all of sudden also
isn't ideal.

I would only advocate for doing this if there were no other changes e.g.
related to R_xlen_t that will cause the same breakage (in that case we
should just rip the bandaid off and take our medicine).

On Sun, Jun 28, 2015 at 11:30 PM, JJ Allaire jj.allaire@gmail.com wrote:

Sounds good. Dirk and I had a discussion regarding whether we should make
this next change one that "breaks" users running with new headers against
old versions of libR.so (a configuration which is not only possible but
likely given the way package updates/dependencies work).

It's obviously cleaner to insist on the integrity of version-synced
headers <-> shared library however it will cause of bunch of (temporary)
breakage in the field if we don't code around it. The way to code around it
would be to look for the static depth counter increment/decrement functions
in the shared library but NOT fail (but rather revert to the old non-depth
counted behavior) if they aren't there.

Let's settle this over email / PR over the next few days.

On Sun, Jun 28, 2015 at 5:10 PM, Kevin Ushey notifications@github.com
wrote:

Merged #313 #313.

—
Reply to this email directly or view it on GitHub
#313 (comment).