Comments (11)
I'm not necessarily opposed to this, but have you seen any problems arise from the commonly-used pattern?
from withr.
Of course I haven't. But how could I? Errors only happen if you manually interrupt between the two expressions, or the first expression is interrupted (e.g. because R runs out of memory) after it created some side effects.
Even if an error happens because of this, most probably you would have no idea what caused it, and of course it is not reproducible.
This is kind of similar to PROTECT bugs that only happen in edge cases. Probably less important, though.
Race conditions like this are considered to be security vulnerabilities in general, but I guess this is not really a serious issue for R, at least currently. But note that as R becomes more mainstream and with increasing embedded use, this might become an issue. E.g. MS plans to embed R into SQL server, and then it might become an issue. Not the on.exit()
calls in devtools
probably, but the general practice of not caring about race conditions.
So, yes, the importance is negligible. But it also does not take much to write things correctly, just switch the order of the two expressions.
from withr.
This is only a problem with interrupts, not errors, right?
from withr.
I think errors might be problematic, too. E.g. if the change_something()
call errors out, and has already changed something before the error (e.g. it has side effects), then you can have a problem.
from withr.
But normally you don't need to cleanup if the setup function fails - it's responsible for doing any intermediate house keeping.
from withr.
Ideally yes. But this is hard to achieve sometimes. E.g. look at devtools:::set_locale
:
function (cats) {
stopifnot(is.named(cats), is.character(cats))
old <- vapply(names(cats), Sys.getlocale, character(1))
mapply(Sys.setlocale, names(cats), cats)
invisible(old)
}
If there is an error in the mapply
, then the locale settings that were already set will not be undone.
from withr.
Does tryCatch + finally have the same problem?
I understand that this is a potential problem, but it seems like a fairly unlikely scenario to me. (i.e. compare to all the other numerous sources of bugs that I could fix, this is fairly low down on my list)
from withr.
Does tryCatch + finally have the same problem?
No, I think finally
is always evaluated, so that is safe.
I understand that this is a potential problem, but it seems like a fairly unlikely scenario to me. (i.e. compare to all the other numerous sources of bugs that I could fix, this is fairly low down on my list)
I understand, I am not doing it either in my projects. (I mean fixing the existing code.) But when you next time write an on.exit()
expression, consider putting it before the expression that changes the global context. :)
from withr.
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/
from withr.
Real-world reprex. This works:
my_set <- function(what) {
old <- Sys.getlocale("LC_COLLATE")
Sys.setlocale("LC_COLLATE", what)
warning("This is deprecated")
invisible(old)
}
my_with <- withr:::with_(my_set)
print(Sys.getlocale("LC_COLLATE"))
#> [1] "en_US.UTF-8"
my_with("de_DE", NULL)
#> NULL
#> Warning messages:
#> 1: In my_set(what = new) : This is deprecated
#> 2: In my_set(old) : This is deprecated
print(Sys.getlocale("LC_COLLATE"))
#> [1] "en_US.UTF-8"
With an early exit this fails:
tryCatch(
my_with("de_DE", NULL),
warning = identity
)
#> <simpleWarning in my_set(what = new): This is deprecated>
print(Sys.getlocale("LC_COLLATE"))
#> [1] "de_DE"
It looks like this is causing the r-devel + windows failures of testthat.
from withr.
Basically the old <- set(new); on.exit(set(old))
idiom (let's call it the "setter restoration pattern") is unsafe and will cause problems in the long run. Since withr functions are used so deeply in our stack, they have to be as safe as they can. #192 progresses towards this goal by providing the mechanisms to implement the "getter pattern" old <- get(); on.exit(set(old))
.
In general, I think Gabor is right that we should avoid setter restoration in critical packages in favour of getter restoration.
from withr.
Related Issues (20)
- defer() run immediately when used inside Rmd HOT 1
- with_language() not working in RStudio IDE for Windows 10 HOT 2
- Something weird with withr 2.5.0 HOT 1
- Problem with `with_db_connection`
- with_trace()? HOT 1
- Should the parameter `tmpdir` be superseded by a new `tempdir` parameter? HOT 2
- local_assign()? HOT 2
- Provide more control over `defer_parent()` messages
- `withr::local_`: error `could not find function` when `withr` is not attached
- How to undo call to `local_dir()` or `local_options()` in global environment? HOT 2
- unclear documentation for lines= argument of local_tempfile
- Support UTF-8 content in local_tempfile(lines = ) HOT 2
- Provide `local_bindings()`? HOT 3
- with_language() fails to reset the cache on Alpine Linux HOT 5
- `local_tempfile()` should use a binary connection for `lines`
- Add short vignette on writing your own withr-like functions
- LIGHT MODE: Move 'stats' from Imports to Suggests HOT 8
- Working directory inside with_dir/local_dir does not match provided new directory
- with_s3_method HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from withr.