[Rcpp-devel] RCPP_USE_UNWIND_PROTECT by default
We can try to take V8 out of the equation, and see what actually causes the change. V8 uses (and tests!) the Rcpp feature to call an R function from C++. This behaves quite differently when using RCPP_UNWIND_PROTECT. Here is a dummy package to demonstrate this: https://github.com/jeroen/uptest The use case is simple: we want to call some R function from C++, and if it fails, catch the error message and deal with it in C++. I suspect there are more packages doing this, but perhaps they are not testing this case. The example from uptest above show that when we compile with RCPP_UNWIND_PROTECT, any R error is always printed directly to the console. Even if we catch the Rcpp::LongjumpException in C++, the R error still seems to bubble up (as Iñaki also noted). I think this is at least surprising. Then I am not sure what we do in C++ now with this Rcpp::LongjumpException if we catch it. We certainly don't want to unwind all the way in the middle of running the javascript code. The JavaScript code should be able to catch the R error, and continue running the script. Anyway if you want to make RCPP_UNWIND_PROTECT the default, I can obviously opt-out in V8, but as an Rcpp user I am not convinced this is an improvement.
On Tue, Feb 1, 2022 at 6:52 PM Dirk Eddelbuettel <edd at debian.org> wrote:
Lionel,
Thanks a bunch for the prompt and detailed reply. CCing Jeroen now, I think I
had poked him over DM on this but not followed. Easy for us to delegate to V8 :)
That is of course a complicated (and toolchain-dependent) package too so we
shall see what we can do there. But the good news on our end is that we can
likely just enable RCPP_UNWIND_PROTECT and everybody (apart from V8) may
indeed be better off.
Cheers, Dirk
On 1 February 2022 at 12:08, Lionel Henry wrote:
| Hi Iñaki,
|
| IIRC, at the time I was concerned it might be undefined behaviour (from R's
| viewpoint) to catch a longjump exception (wich wraps an R unwind token) without
| rethrowing it. I think this is why I didn't have `LongjumpException` inherit
| from `std::exception` because there seemed to be an assumption that an exception
| inheriting from that class can be arbitrarily caught, and perhaps reported in
| some way to users or callers via the `what()` method.
|
| A longjump token may represent different kinds of long jumps. If it represents
| an error, it might be reasonable to catch the token and recover. However it can
| also represent a restart invokation, a condition being caught by another handler
| upstack, or a long return (see `base::callCC()`). For these cases the R language
| does not provide a way of interrupting the unwinding. Except, maybe, through
| `on.exit()` expressions on the stack which can throw or long-return, and thus
| overtake the current jump.
|
| Whether it's UB or not could be checked with Luke. That said, even if well
| defined, I would generally not recommand catching a longjump token and transform
| it to an unknown exception. In general it'd be better to resume the unwinding
| with the token. As mentioned above, it does not necessarily represent an error
| or abnormal situation.
|
| I'm not familiar with V8 but I took a quick look. IIUC R is called back from JS
| through Rcpp? E.g. via `console.r.call()`? And it looks like throwing exceptions
| (or longjumping) over the JS stack is UB. So I think the proper thing to do is
| to catch `Rcpp::LongjumpException()`, then unwind the JS stack, probably with
| `v8::ThrowException()`, but the longjump exception (or at least the unwind token
| it wraps) needs to be somehow communicated to the other side. Once back in a
| safe Rcpp context with no JS on the stack, resume the longjump with the
| exception or token.
|
| Of course it's easier to just disable unwind-protection in V8. But implementing
| the approach above would not only improve performance, it would also improve the
| semantics of V8 callbacks to R.
|
| Best,
| Lionel
|
|
| On 1/31/22, Iñaki Ucar <iucar at fedoraproject.org> wrote:
| > Hi Lionel,
| >
| > I've been setting this for years in my own packages, and particularly
| > in a simulator that greatly benefits from this due to its heavy usage
| > of R calls from the C++ simulation loop. Now, we're looking into
| > setting this feature on by default in the next release of Rcpp to
| > improve the performance of other packages, such as httpuv [1], as
| > well.
| >
| > Dirk has run a full revdep check that looks very promising, with just
| > one new failure (the V8 package), and it would be great if you could
| > take a look when you have time. It seems that the UNWIND_PROTECT
| > mechanism somehow collides with v8's exception handling:
| >
| >> test_check("V8")
| > terminate called after throwing an instance of 'Rcpp::LongjumpException'
| > Aborted
| >
| > V8 sees this exception, doesn't know what to do with it, and then the
| > program is aborted. This is kind of a Russian Doll, and probably an
| > extreme case, but we are wondering whether we can do something on the
| > Rcpp side to support v8's engine. V8 can always undefine
| > UNWIND_PROTECT if we ship this by default, but this is clearly one of
| > those packages that could benefit from the improved performance that
| > this feature enables.
| >
| > So far, I found that Rcpp::LongjumpException is the only exception in
| > Rcpp that doesn't inherit from std::exception (which is what V8
| > expects). Is this intended/required? I checked that, if
| > LongjumpException inherits from std::exception, V8 doesn't crash there
| > (the exception is recognized), but then two errors are shown in the R
| > console, which is not ideal either:
| >
| >> ctx <- V8::v8()
| >> ctx$eval("console.r.eval('doesnotexists')")
| > Error in eval(ei, envir) : object 'doesnotexists' not found
| > Error: std::exception
| >> x <- try(ctx$eval("console.r.eval('doesnotexists')"))
| > Error : std::exception
| >> x
| > [1] "Error : std::exception\n"
| > attr(,"class")
| > [1] "try-error"
| > attr(,"condition")
| > <std::runtime_error: std::exception>
| >
| > Please let us know if you have some time for this. Any help would be
| > appreciated.
| >
| > Cheers,
| > Iñaki
| >
| > [1] https://github.com/rstudio/httpuv/issues/244
| >
| > --
| > Iñaki Úcar
| >
--
https://dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org