{"action":"created","issue":{"url":"https://api.github.com/repos/RcppCore/Rcpp/issues/323","labels_url":"https://api.github.com/repos/RcppCore/Rcpp/issues/323/labels{/name}","comments_url":"https://api.github.com/repos/RcppCore/Rcpp/issues/323/comments","events_url":"https://api.github.com/repos/RcppCore/Rcpp/issues/323/events","html_url":"https://github.com/RcppCore/Rcpp/pull/323","id":94831014,"number":323,"title":"remove calling handlers from Rcpp_eval","user":{"login":"kevinushey","id":1976582,"avatar_url":"https://avatars.githubusercontent.com/u/1976582?v=3","gravatar_id":"","url":"https://api.github.com/users/kevinushey","html_url":"https://github.com/kevinushey","followers_url":"https://api.github.com/users/kevinushey/followers","following_url":"https://api.github.com/users/kevinushey/following{/other_user}","gists_url":"https://api.github.com/users/kevinushey/gists{/gist_id}","starred_url":"https://api.github.com/users/kevinushey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kevinushey/subscriptions","organizations_url":"https://api.github.com/users/kevinushey/orgs","repos_url":"https://api.github.com/users/kevinushey/repos","events_url":"https://api.github.com/users/kevinushey/events{/privacy}","received_events_url":"https://api.github.com/users/kevinushey/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2015-07-13T23:04:02Z","updated_at":"2015-07-14T17:54:33Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RcppCore/Rcpp/pulls/323","html_url":"https://github.com/RcppCore/Rcpp/pull/323","diff_url":"https://github.com/RcppCore/Rcpp/pull/323.diff","patch_url":"https://github.com/RcppCore/Rcpp/pull/323.patch"},"body":"(Sorry for the noisy diff -- the gist of this PR is that we no longer evaluate with `withCallingHandlers`; instead just with `tryCatch`).\r\n\r\nThis PR supercedes https://github.com/RcppCore/Rcpp/pull/319. The main idea:\r\n\r\n1. It appears that when `options(warn = 2)` is set, the warning is converted to an error before escaping the `tryCatch` call and so `tryCatch` handles it properly,\r\n\r\n2. When we attach calling handlers to the call, recursive evaluation of R expressions causes loads of problems that make it very difficult to properly forward errors. It looks like R 'does the right thing' when we don't attach calling handlers, so I believe it is safe to just remove them.\r\n\r\nAFAIK, a `warning` (or `message`) call hence will not  `longjmp` out of the current context -- only R errors will do that. I think any handler that attempts to call `stop()` from R code will still get caught by our top-level `tryCatch` and so I think we are safe.\r\n\r\nThe only problem I've found thus far is that messages called using the Rcpp function class are not properly suppressed, e.g. given this:\r\n\r\n```cpp\r\n#include <Rcpp.h>\r\nusing namespace Rcpp;\r\n\r\n// [[Rcpp::export]]\r\nvoid test() {\r\n    \r\n    Function msg(\"message\");\r\n    msg(\"Rcpp message\");\r\n    \r\n    Shield<SEXP> messageSEXP(Rf_mkString(\"R message\"));\r\n    Shield<SEXP> messageCall(Rf_lang2(\r\n        Rf_install(\"message\"),\r\n        messageSEXP\r\n    ));\r\n    \r\n    Rf_eval(messageCall, R_GlobalEnv);\r\n}\r\n\r\n// [[Rcpp::export]]\r\nvoid exec(Function f) { f(); }\r\n\r\n/*** R\r\ntest()\r\nsuppressMessages(test())\r\n*/\r\n```\r\n\r\nI get:\r\n\r\n```\r\n> Rcpp::sourceCpp('~/Untitled.cpp')\r\n\r\n> test()\r\nRcpp message\r\nR message\r\n\r\n> suppressMessages(test())\r\nRcpp message\r\n```\r\n\r\nSo I believe this is an artefact of how the `Rcpp::Function` class is written and not of this evaluation mechanism."},"comment":{"url":"https://api.github.com/repos/RcppCore/Rcpp/issues/comments/121322730","html_url":"https://github.com/RcppCore/Rcpp/pull/323#issuecomment-121322730","issue_url":"https://api.github.com/repos/RcppCore/Rcpp/issues/323","id":121322730,"user":{"login":"kevinushey","id":1976582,"avatar_url":"https://avatars.githubusercontent.com/u/1976582?v=3","gravatar_id":"","url":"https://api.github.com/users/kevinushey","html_url":"https://github.com/kevinushey","followers_url":"https://api.github.com/users/kevinushey/followers","following_url":"https://api.github.com/users/kevinushey/following{/other_user}","gists_url":"https://api.github.com/users/kevinushey/gists{/gist_id}","starred_url":"https://api.github.com/users/kevinushey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kevinushey/subscriptions","organizations_url":"https://api.github.com/users/kevinushey/orgs","repos_url":"https://api.github.com/users/kevinushey/repos","events_url":"https://api.github.com/users/kevinushey/events{/privacy}","received_events_url":"https://api.github.com/users/kevinushey/received_events","type":"User","site_admin":false},"created_at":"2015-07-14T17:54:33Z","updated_at":"2015-07-14T17:54:33Z","body":"Thinking about this a bit more, I think that either `tryCatch` or a lower-level handler attachment through `.Internal(.addCondHands(...))` is the correct solution here.\r\n\r\nThis PR currently works for all evaluation that happens through `Rcpp_eval` -- we are protected from `longjmp`s as the `tryCatch` handlers will catch those errors before a `longjmp` to top level can occur. If we wanted to avoid the overhead of `tryCatch`, we could just mock the same process using `.Internal(.addCondHands(...))`.\r\n\r\nThe other option is to use `R_ToplevelExec`, but the problem with that function is that is also detaches all existing handlers (something undesired for people who want to emit / control message and warning output from Rcpp function calls, which we've definitely seen). Since we _can_ capture both errors and interrupts using R handlers (thereby avoiding `longjmp`s) I think that's preferable to using `R_ToplevelExec`.\r\n\r\n(Of course, if we have access to R's contexts, we could use `R_ToplevelExec` to create a top-level context and then re-attach existing handlers -- this is what @romainfrancois has effectively done in Rcpp11 here: https://github.com/Rcpp11/Rcpp11/blob/master/inst/include/Rcpp/Context.h#L92-L121)\r\n\r\nThe error I was thinking of before -- inability to catch top-level calls to `Rf_error()` -- is something that I don't think we can protect users from (ie, if a user attempts to call a `longjmp`ing R API function, they either need to make sure the evaluation is 'protected' with `Rcpp_eval` or use `tryCatch` / `R_ToplevelExec` themselves). There are a couple 'blunt' shields we could use, however:\r\n\r\n- Redefine `Rf_error()` to `Rcpp::stop` -- this should be okay since anyone using Rcpp will be using it in a C++ compilation unit, and so any calls to `Rf_error()` would effectively be 'wrong',\r\n\r\n- Add an `executeSafely` function that takes a lambda function and performs a `tryCatch` or `R_ToplevelExec` to ensure any R API functions called can't `longjmp` out of the current executing context.\r\n"}}