I have pushed a new version of V8 to CRAN that wraps the R callbacks in another tryCatch() such that we do not make any assumptions on the sort of exception handing provided by Rcpp. This should unblock you if you want to move forward on this change.
On Sun, Feb 6, 2022 at 9:30 PM Lionel Henry <lio...@rstudio.com> wrote: > > A few points: > > - As argued in my previous mail, a longjump exception does not necessarily > represent an error and doesn't always represent something that should be > allowed to be caught. If an R function invokes a restart or a long return, > or > more commonly if a condition handler upstack catches a condition emitted by > the R callback, C++ code shouldn't regain control during the unwind. > > - If you want to catch R errors, I think the proper way is to wrap the > callback > in `tryCatch()` and convert to an `std::exception`. The interface for this > is > `Rcpp_eval()`. > > - It may be that `Rcpp::function` calls should use `Rcpp_eval()` instead of > `Rcpp_fast_eval()`. They will be slower but will properly emit catchable > `std::exception` in case of errors or interrupts. > > - Even if you use `Rcpp_eval()`, longjump exceptions might still be thrown > when > the long jump happens for another reason, e.g. when an R function upstack > catches a condition. These shouldn't be caught. > > - When a longjump exception is encountered, I think that V8 should unwind the > JS > stack and thread the unwind token back to a safe place and resume the jump. > > Best, > Lionel > > > On 2/6/22, Jeroen Ooms <jeroeno...@gmail.com> wrote: > > 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 <e...@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 <iu...@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 | e...@debian.org > > _______________________________________________ Rcpp-devel mailing list Rcpp-devel@lists.r-forge.r-project.org https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel
