Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use built-in callback cancellation from later #115

Merged
merged 2 commits into from Dec 11, 2020
Merged

Use built-in callback cancellation from later #115

merged 2 commits into from Dec 11, 2020

Conversation

wch
Copy link
Collaborator

@wch wch commented Dec 10, 2020

Closes #114.

Previously, instead of cancelling the callback immediately, the canceller function would simply assign func <<- NULL. That would make the actual callback (which in turn calls func()) effectively a no-op, but it would still exist and consume memory.

This change uses the built-in later callback cancellation added in 1.0.0.

With this change, the test code from #114 (comment) does not leak.

library(pryr)
library(pool)
library(dplyr)

pool <- dbPool(RSQLite::SQLite(), dbname = ":memory:")

# Use dplyr syntax to copy mtcars to object
db_desc(pool)
copy_to(pool, mtcars, "mtcars", temporary = FALSE)

old.mem <- pryr::mem_used()
highest.mem <- old.mem
when <- Sys.time()
i <- 0
while (TRUE) {
  Sys.sleep(0.5)
  x <- dbGetQuery(pool, "SELECT * FROM mtcars LIMIT 5;")
  new.mem <- pryr::mem_used()
  if (new.mem > highest.mem) {
    highest.mem <- new.mem
    when <- as.character(Sys.time())
  }
  diff <- new.mem - old.mem
  old.mem <- new.mem
  
  cat(as.character(Sys.time()), ' Highest: ', highest.mem, ' at ', when, '. Mem now: ', new.mem, ' (', diff, ')\n', sep='')
}

Output:

2020-12-10 10:14:35 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095472 (11408)

2020-12-10 10:14:35 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095312 (-160)
2020-12-10 10:14:36 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (120)
2020-12-10 10:14:36 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095368 (-64)
2020-12-10 10:14:37 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (64)
2020-12-10 10:14:37 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:38 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:39 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:39 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:40 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:40 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:41 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)
2020-12-10 10:14:41 Highest: 79095472 at 2020-12-10 10:14:35. Mem now: 79095432 (0)

Note: Using the CRAN version of pool (0.1.5), I tried using a smaller idleTimeout, as in the following:

pool <- dbPool(RSQLite::SQLite(), dbname = ":memory:", idleTimeout = 2)

I expected it to release the memory after 2 seconds, but that doesn't seem to be the case, and I'm not sure why.

@wch wch requested a review from jcheng5 December 10, 2020 16:16
@jcheng5 jcheng5 merged commit 0d059e4 into master Dec 11, 2020
@jcheng5 jcheng5 deleted the fix-leak branch December 11, 2020 06:57
@jcheng5
Copy link
Member

jcheng5 commented Dec 11, 2020

Nice work @wch!

@wch
Copy link
Collaborator Author

wch commented Dec 11, 2020

Oh, I know why the callback was never invoked even with the shorter timeout: later::run_now() is never called and the console isn't idle, so there's not an opportunity for the callbacks to be invoked. That shouldn't be a problem in a Shiny app, though.

In a Shiny app, when pool is used with the default settings, it should release the memory after the 60 seconds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in Shiny app when using pool
2 participants