on.exit calls

[r-lib]

on.exit calls

[wch]

I'm not necessarily opposed to this, but have you seen any problems arise from the commonly-used pattern?

[gaborcsardi]

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.

[hadley]

This is only a problem with interrupts, not errors, right?

[gaborcsardi]

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.

[hadley]

But normally you don't need to cleanup if the setup function fails - it's responsible for doing any intermediate house keeping.

[gaborcsardi]

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.

[hadley]

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)

[gaborcsardi]

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. :)

[lock]

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/

[lionel-]

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.

[lionel-]

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.