From 8c19450b10d26570d0cd593d431a5ca7275dd886 Mon Sep 17 00:00:00 2001
From: Winston Chang <winston@stdout.org>
Date: Mon, 26 Aug 2019 20:58:23 -0500
Subject: [PATCH] Use safer method to remove observer

---
 R/reactives.R                    | 17 +++++++++++-----
 tests/testthat/test-reactivity.r | 34 +++++++++++++++++---------------
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/R/reactives.R b/R/reactives.R
index bc93cb659..64936f3ad 100644
--- a/R/reactives.R
+++ b/R/reactives.R
@@ -1733,7 +1733,17 @@ reactivePoll <- function(intervalMillis, session, checkFunc, valueFunc) {
 
   rv <- reactiveValues(cookie = isolate(checkFunc()))
 
+  re_finalized <- FALSE
+
   o <- observe({
+    # When no one holds a reference to the reactive returned from
+    # reactivePoll, destroy and remove the observer so that it doesn't keep
+    # firing and hold onto resources.
+    if (re_finalized) {
+      o$destroy()
+      rm(o, envir = parent.env(environment()))
+    }
+
     rv$cookie <- checkFunc()
     invalidateLater(intervalMillis(), session)
   })
@@ -1746,11 +1756,8 @@ reactivePoll <- function(intervalMillis, session, checkFunc, valueFunc) {
 
   }, label = NULL)
 
-  # When no one holds a reference to this object anymore, destroy and remove the
-  # observer so that it doesn't keep firing, and hold onto resources.
-  safe_finalizer(attr(re, "observable"), function(e) {
-    o$destroy()
-    rm(o, envir = parent.env(environment()))
+  reg.finalizer(attr(re, "observable"), function(e) {
+    re_finalized <<- TRUE
   })
 
   # So that the observer and finalizer function don't (indirectly) hold onto a
diff --git a/tests/testthat/test-reactivity.r b/tests/testthat/test-reactivity.r
index bf406cc0c..bc37349a8 100644
--- a/tests/testthat/test-reactivity.r
+++ b/tests/testthat/test-reactivity.r
@@ -1339,31 +1339,33 @@ test_that("reactivePoll doesn't leak observer (#1548)", {
   )
 
   observe({
-    message(count())
+    count()
   })
-  while(i < 3) {
+
+  while (i < 3) {
     Sys.sleep(0.05)
     shiny:::timerCallbacks$executeElapsed()
     shiny:::flushReact()
   }
 
   # Removing the reference to count means that no one can use it anymore, and so
-  # the checkFunc should not keep firing.
+  # the finalizer should run. The finalizer sets a flag which will allow the
+  # observer (which calls `checkFunc`) to run one more time; in that run, it
+  # will remove itself.
   rm(count)
   gc()
 
-  # Make sure the finalizer from safe_finalizer runs -- it is scheduled using
-  # later().
-  later::run_now()
+  # If the reactivePoll was cleaned up, then the first run of this loop will
+  # increment i (bringing its value to 4), but in that run, the observer will
+  # remove itself so subsequent runs will no longer run `checkFunc`.
+  for (n in 1:3) {
+    Sys.sleep(0.05)
+    shiny:::timerCallbacks$executeElapsed()
+    shiny:::flushReact()
+  }
 
-  # If the reactivePoll was cleaned up, then running the following should no
-  # longer increment i.
-  Sys.sleep(0.05)
-  shiny:::timerCallbacks$executeElapsed()
-  shiny:::flushReact()
-  Sys.sleep(0.05)
-  shiny:::timerCallbacks$executeElapsed()
-  shiny:::flushReact()
-
-  expect_equal(i, 3L)
+  # It is possible in the future we will have a better method which will remove
+  # the observer without requiring the one extra run. If that happens, the
+  # expected value will be 3 instead of 4.
+  expect_equal(i, 4L)
 })