From 777745ca889e71b07eb306464aaaa6ea7fe9956f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 12 Jan 2023 17:59:48 -0600 Subject: [PATCH 01/12] Improve validation * Correctly access pool metadata, respecting `validateQuery` and ensuring cache actually works. * Validate on creation to ensure problems are revealed as soon as possible. Fixes #133 --- R/DBI-pool.R | 2 +- R/pool.R | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/R/DBI-pool.R b/R/DBI-pool.R index 81b88e0..84b29bd 100644 --- a/R/DBI-pool.R +++ b/R/DBI-pool.R @@ -24,7 +24,7 @@ setMethod("onDestroy", "DBIConnection", function(object) { #' @export #' @rdname object setMethod("onValidate", "DBIConnection", function(object) { - pool <- attr(object, "..metadata", exact = TRUE)$pool + pool <- attr(object, "pool_metadata", exact = TRUE)$pool query <- pool$state$validateQuery if (!is.null(query)) { diff --git a/R/pool.R b/R/pool.R index b5977a1..f156736 100644 --- a/R/pool.R +++ b/R/pool.R @@ -171,7 +171,12 @@ Pool <- R6::R6Class("Pool", pool_metadata$pool <- self pool_metadata$valid <- TRUE pool_metadata$state <- NULL - pool_metadata$lastValidated <- NULL + pool_metadata$lastValidated <- Sys.time() + + # Now that metadata has been setup we can validate it + # This ensures that any problems bubble up immediately, not after + # validationInterval seconds + onValidate(object) ## detect leaked connections and destroy them reg.finalizer(pool_metadata, function(e) { From f3dd7fe8c3ae6f5cb05f87f7d9a2a99a386a81c7 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 12 Jan 2023 18:01:41 -0600 Subject: [PATCH 02/12] Standardise validation query formats --- R/DBI-pool.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/R/DBI-pool.R b/R/DBI-pool.R index 84b29bd..00c939e 100644 --- a/R/DBI-pool.R +++ b/R/DBI-pool.R @@ -40,12 +40,12 @@ setMethod("onValidate", "DBIConnection", function(object) { "SELECT 1", "SELECT 1 FROM DUAL", "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS", - "SELECT * FROM INFORMATION_SCHEMA.TABLES", + "SELECT 1 FROM INFORMATION_SCHEMA.TABLES", "VALUES 1", "SELECT 1 FROM SYSIBM.SYSDUMMY1", - "select count(*) from systables", - "select count(*) from SYS_TABLES", - "select count(*) from SYS.TABLES" + "SELECT 1 FROM systables", + "SELECT 1 FROM SYS_TABLES", + "SELECT 1 FROM SYS.TABLES" ) ## Iterates through the possible validation queries: From 62fba23ba8fa72027bc4e07e7cb7790ea2697ee3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 11:24:49 -0600 Subject: [PATCH 03/12] Use alternative strategy to force validation --- R/pool.R | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/R/pool.R b/R/pool.R index f156736..a85459c 100644 --- a/R/pool.R +++ b/R/pool.R @@ -171,12 +171,8 @@ Pool <- R6::R6Class("Pool", pool_metadata$pool <- self pool_metadata$valid <- TRUE pool_metadata$state <- NULL - pool_metadata$lastValidated <- Sys.time() - - # Now that metadata has been setup we can validate it - # This ensures that any problems bubble up immediately, not after - # validationInterval seconds - onValidate(object) + # force validation to happen immediately to surface any issues + pool_metadata$lastValidated <- Sys.time() - self$validationInterval - 1 ## detect leaked connections and destroy them reg.finalizer(pool_metadata, function(e) { From 2236166e05dc506b798090727b32cdf9419dba24 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 11:36:15 -0600 Subject: [PATCH 04/12] Test that validateQuery is cached --- tests/testthat/test-DBI-pool.R | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 tests/testthat/test-DBI-pool.R diff --git a/tests/testthat/test-DBI-pool.R b/tests/testthat/test-DBI-pool.R new file mode 100644 index 0000000..84a374e --- /dev/null +++ b/tests/testthat/test-DBI-pool.R @@ -0,0 +1,10 @@ +test_that("onValidate() caches query", { + pool <- local_pool() + # reset cache from initial creation + validation + pool$state$validateQuery <- NULL + + con <- poolCheckout(pool) + on.exit(poolReturn(con)) + onValidate(con) + expect_equal(pool$state$validateQuery, "SELECT 1") +}) From eef06dd6f10f3bb20a3df72a52443d2d2121cbad Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 13:07:44 -0600 Subject: [PATCH 05/12] Use helper for accessing pool metadata --- DESCRIPTION | 2 +- R/DBI-pool.R | 2 +- R/pool-methods.R | 6 +----- R/pool.R | 17 +++++++---------- tests/testthat/_snaps/release.md | 8 ++++++++ tests/testthat/test-fetch.R | 10 +++++----- tests/testthat/test-release.R | 10 +++------- tests/testthat/utils.R | 21 +++++++++++++++++++++ 8 files changed, 47 insertions(+), 29 deletions(-) create mode 100644 tests/testthat/_snaps/release.md diff --git a/DESCRIPTION b/DESCRIPTION index aa34777..2937b09 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -21,7 +21,7 @@ Imports: DBI, later (>= 1.0.0), R6, - rlang + rlang (>= 1.0.0) Suggests: covr, dbplyr, diff --git a/R/DBI-pool.R b/R/DBI-pool.R index 00c939e..6d7a5f0 100644 --- a/R/DBI-pool.R +++ b/R/DBI-pool.R @@ -24,7 +24,7 @@ setMethod("onDestroy", "DBIConnection", function(object) { #' @export #' @rdname object setMethod("onValidate", "DBIConnection", function(object) { - pool <- attr(object, "pool_metadata", exact = TRUE)$pool + pool <- pool_metadata(object)$pool query <- pool$state$validateQuery if (!is.null(query)) { diff --git a/R/pool-methods.R b/R/pool-methods.R index 87aa590..0f4619a 100644 --- a/R/pool-methods.R +++ b/R/pool-methods.R @@ -100,11 +100,7 @@ setGeneric("poolReturn", function(object) { #' @export #' @rdname poolCheckout setMethod("poolReturn", "ANY", function(object) { - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) - if (is.null(pool_metadata) || !pool_metadata$valid) { - stop("Invalid object.") - } - pool <- pool_metadata$pool + pool <- pool_metadata(object)$pool pool$release(object) }) diff --git a/R/pool.R b/R/pool.R index a85459c..84d8fb5 100644 --- a/R/pool.R +++ b/R/pool.R @@ -67,13 +67,10 @@ Pool <- R6::R6Class("Pool", ## (sets up task to destroy the object if the number of ## total objects exceeds the minimum) release = function(object) { - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(object) if (pool_metadata$state == "free") { stop("This object was already returned to the pool.") } - if (is.null(pool_metadata) || !pool_metadata$valid) { - stop("Invalid object.") - } ## immediately destroy object if pool has already been closed if (!self$valid) { private$changeObjectStatus(object, NULL) @@ -176,8 +173,8 @@ Pool <- R6::R6Class("Pool", ## detect leaked connections and destroy them reg.finalizer(pool_metadata, function(e) { - if (pool_metadata$valid) { - warning("You have a leaked pooled object.") + if (e$valid) { + warning("You have a leaked pooled object.", call. = FALSE) } }, onexit = TRUE) @@ -188,7 +185,7 @@ Pool <- R6::R6Class("Pool", ## tries to run onDestroy destroyObject = function(object) { tryCatch({ - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(object, check_valid = FALSE) if (!pool_metadata$valid) { warning("Object was destroyed twice.") return() @@ -210,7 +207,7 @@ Pool <- R6::R6Class("Pool", ## gets taken and vice versa. Valid values for `from` ## and `to` are: NULL, "free", "taken" changeObjectStatus = function(object, to) { - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(object) id <- pool_metadata$id from <- pool_metadata$state @@ -249,7 +246,7 @@ Pool <- R6::R6Class("Pool", }, cancelScheduledTask = function(object, task) { - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(object, check_valid = FALSE) taskHandle <- pool_metadata[[task]] if (!is.null(taskHandle)) { pool_metadata[[task]] <- NULL @@ -297,7 +294,7 @@ Pool <- R6::R6Class("Pool", ## secs have passed since the last validation (this allows ## us some performance gains) validate = function(object) { - pool_metadata <- attr(object, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(object) lastValidated <- pool_metadata$lastValidated ## if the object has never been validated, set `lastValidated` ## to guarantee that it will be validated now diff --git a/tests/testthat/_snaps/release.md b/tests/testthat/_snaps/release.md new file mode 100644 index 0000000..c228c53 --- /dev/null +++ b/tests/testthat/_snaps/release.md @@ -0,0 +1,8 @@ +# poolReturn() errors if object is not valid + + Code + poolReturn("x") + Condition + Error in `poolReturn()`: + ! `object` is not an pooled object. + diff --git a/tests/testthat/test-fetch.R b/tests/testthat/test-fetch.R index f5238ef..68070d6 100644 --- a/tests/testthat/test-fetch.R +++ b/tests/testthat/test-fetch.R @@ -33,14 +33,14 @@ describe("fetch", { it("only validates after validationInterval", { obj <- poolCheckout(pool) t0 <- Sys.time() - pool_metadata <- attr(obj, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(obj) lastValidated_t0 <- pool_metadata$lastValidated poolReturn(obj) obj <- poolCheckout(pool) t1 <- Sys.time() - pool_metadata <- attr(obj, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(obj) lastValidated_t1 <- pool_metadata$lastValidated if (difftime(t1, t0, units = "secs") < pool$validationInterval) { @@ -54,7 +54,7 @@ describe("fetch", { obj <- poolCheckout(pool) t2 <- Sys.time() - pool_metadata <- attr(obj, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(obj) lastValidated_t2 <- pool_metadata$lastValidated if (difftime(t2, t0, units = "secs") < pool$validationInterval) { @@ -70,7 +70,7 @@ describe("fetch", { obj <- poolCheckout(pool) t3 <- Sys.time() - pool_metadata <- attr(obj, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(obj) lastValidated_t3 <- pool_metadata$lastValidated if (difftime(t3, t0, units = "secs") > pool$validationInterval) { @@ -84,7 +84,7 @@ describe("fetch", { obj <- poolCheckout(pool) t4 <- Sys.time() - pool_metadata <- attr(obj, "pool_metadata", exact = TRUE) + pool_metadata <- pool_metadata(obj) lastValidated_t4 <- pool_metadata$lastValidated if (difftime(t4, t3, units = "secs") < pool$validationInterval) { diff --git a/tests/testthat/test-release.R b/tests/testthat/test-release.R index 7ef2a75..eb4495a 100644 --- a/tests/testthat/test-release.R +++ b/tests/testthat/test-release.R @@ -36,11 +36,6 @@ describe("release", { checkCounts(pool, free = 1, taken = 0) }) - it("throws if object is not valid", { - obj <- "a" - expect_error(poolReturn(obj), "Invalid object.") - }) - it("warns if onPassivate fails", { checkCounts(pool, free = 1, taken = 0) obj <- poolCheckout(pool) @@ -66,5 +61,6 @@ describe("release", { }) }) - - +test_that("poolReturn() errors if object is not valid", { + expect_snapshot(poolReturn("x"), error = TRUE) +}) diff --git a/tests/testthat/utils.R b/tests/testthat/utils.R index f8c74fa..a389b63 100644 --- a/tests/testthat/utils.R +++ b/tests/testthat/utils.R @@ -1,3 +1,24 @@ +pool_metadata <- function(x, + check_valid = TRUE, + error_call = caller_env(), + error_arg = caller_arg(x)) { + meta <- attr(x, "pool_metadata", exact = TRUE) + + if (is.null(meta)) { + abort( + paste0("`", error_arg, "` is not an pooled object."), + call = error_call + ) + } + if (check_valid && !meta$valid) { + abort( + paste0("`", error_arg, "` is no longer valid."), + call = error_call + ) + } + + meta +} #********************************************************************# #********** Create Mock Pooled object for testing purposes **********# From 4bfc67f2e37735fe9ded03f86fd2e690df2d7032 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 13:08:25 -0600 Subject: [PATCH 06/12] Use withr::defer() to correct order of closing --- tests/testthat/test-DBI-pool.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-DBI-pool.R b/tests/testthat/test-DBI-pool.R index 84a374e..c550c4e 100644 --- a/tests/testthat/test-DBI-pool.R +++ b/tests/testthat/test-DBI-pool.R @@ -4,7 +4,7 @@ test_that("onValidate() caches query", { pool$state$validateQuery <- NULL con <- poolCheckout(pool) - on.exit(poolReturn(con)) + withr::defer(poolReturn(con)) onValidate(con) expect_equal(pool$state$validateQuery, "SELECT 1") }) From 41d6435d271616e6f8bc9b353d503d24993447ed Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 13:09:45 -0600 Subject: [PATCH 07/12] Restrict query size --- R/DBI-pool.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/R/DBI-pool.R b/R/DBI-pool.R index 6d7a5f0..7248c0d 100644 --- a/R/DBI-pool.R +++ b/R/DBI-pool.R @@ -39,13 +39,13 @@ setMethod("onValidate", "DBIConnection", function(object) { options <- c( "SELECT 1", "SELECT 1 FROM DUAL", - "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS", - "SELECT 1 FROM INFORMATION_SCHEMA.TABLES", + "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS WHERE 0=1", + "SELECT 1 FROM INFORMATION_SCHEMA.TABLES WHERE 0=1", "VALUES 1", - "SELECT 1 FROM SYSIBM.SYSDUMMY1", - "SELECT 1 FROM systables", - "SELECT 1 FROM SYS_TABLES", - "SELECT 1 FROM SYS.TABLES" + "SELECT 1 FROM SYSIBM.SYSDUMMY1 WHERE 0=1", + "SELECT 1 FROM systables WHERE 0=1", + "SELECT 1 FROM SYS_TABLES WHERE 0=1", + "SELECT 1 FROM SYS.TABLES WHERE 0=1" ) ## Iterates through the possible validation queries: From 9d76b4984341a9ab40e7f00522ffe0fb394eb4db Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 13:12:03 -0600 Subject: [PATCH 08/12] Add news bullets --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index f44bcee..2354571 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,10 @@ # pool (development version) +* `dbPool()`'s `validateQuery` is now actually used (#153). + +* Connections are now validated once on first checkout to ensure that the + connection and validation strategy are both ok. + * Added support for SAP HANA databases (@marcosci, #103). # pool 0.1.6 From f75270374abcd411a5cb76e68b809040f4617757 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 14:03:11 -0600 Subject: [PATCH 09/12] Move helper to correct place --- DESCRIPTION | 1 + R/utils.R | 22 ++++++++++++++++++++++ tests/testthat/utils.R | 22 ---------------------- 3 files changed, 23 insertions(+), 22 deletions(-) create mode 100644 R/utils.R diff --git a/DESCRIPTION b/DESCRIPTION index 2937b09..45e3ec9 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -50,6 +50,7 @@ Collate: 'pool-package.R' 'pool.R' 'scheduler.R' + 'utils.R' 'zzz.R' Config/testthat/edition: 3 Config/Needs/website: tidyverse/tidytemplate diff --git a/R/utils.R b/R/utils.R new file mode 100644 index 0000000..386832e --- /dev/null +++ b/R/utils.R @@ -0,0 +1,22 @@ +pool_metadata <- function(x, + check_valid = TRUE, + error_call = caller_env(), + error_arg = caller_arg(x)) { + meta <- attr(x, "pool_metadata", exact = TRUE) + + if (is.null(meta)) { + abort( + paste0("`", error_arg, "` is not an pooled object."), + call = error_call + ) + } + if (check_valid && !meta$valid) { + abort( + paste0("`", error_arg, "` is no longer valid."), + call = error_call + ) + } + + meta +} + diff --git a/tests/testthat/utils.R b/tests/testthat/utils.R index a389b63..598355d 100644 --- a/tests/testthat/utils.R +++ b/tests/testthat/utils.R @@ -1,25 +1,3 @@ -pool_metadata <- function(x, - check_valid = TRUE, - error_call = caller_env(), - error_arg = caller_arg(x)) { - meta <- attr(x, "pool_metadata", exact = TRUE) - - if (is.null(meta)) { - abort( - paste0("`", error_arg, "` is not an pooled object."), - call = error_call - ) - } - if (check_valid && !meta$valid) { - abort( - paste0("`", error_arg, "` is no longer valid."), - call = error_call - ) - } - - meta -} - #********************************************************************# #********** Create Mock Pooled object for testing purposes **********# #********************************************************************# From 94cc8e84c254434cba1d09c4f756325c8d947d1d Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 13 Jan 2023 14:06:39 -0600 Subject: [PATCH 10/12] Add comments to query options --- R/DBI-pool.R | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/R/DBI-pool.R b/R/DBI-pool.R index 7248c0d..782f5b7 100644 --- a/R/DBI-pool.R +++ b/R/DBI-pool.R @@ -37,13 +37,18 @@ setMethod("onValidate", "DBIConnection", function(object) { ## options mostly gathered from here: ## http://stackoverflow.com/a/3670000/6174455 options <- c( + # Most modern databases "SELECT 1", + # Oracle: https://en.wikipedia.org/wiki/DUAL_table "SELECT 1 FROM DUAL", + # HSQLDB "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS WHERE 0=1", "SELECT 1 FROM INFORMATION_SCHEMA.TABLES WHERE 0=1", - "VALUES 1", - "SELECT 1 FROM SYSIBM.SYSDUMMY1 WHERE 0=1", + # DB2: https://www.ibm.com/docs/en/db2-for-zos/12?topic=tables-sysdummy1 + "SELECT 1 FROM SYSIBM.SYSDUMMY1", + # informix "SELECT 1 FROM systables WHERE 0=1", + # SAP HANA "SELECT 1 FROM SYS_TABLES WHERE 0=1", "SELECT 1 FROM SYS.TABLES WHERE 0=1" ) From 348cbeb05c8e1087af72816878bcc339a2bef7e6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sat, 28 Jan 2023 17:18:56 -0600 Subject: [PATCH 11/12] Improve scope of tryCatch() --- R/pool.R | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/R/pool.R b/R/pool.R index a14b558..21c19bd 100644 --- a/R/pool.R +++ b/R/pool.R @@ -201,15 +201,17 @@ Pool <- R6::R6Class("Pool", ## tries to run onDestroy destroyObject = function(object) { + pool_metadata <- pool_metadata(object, check_valid = FALSE) + if (!pool_metadata$valid) { + pool_warn("Object was destroyed twice.") + return() + } + + pool_metadata$valid <- FALSE + private$cancelScheduledTask(object, "validateHandle") + private$cancelScheduledTask(object, "destroyHandle") + tryCatch({ - pool_metadata <- pool_metadata(object, check_valid = FALSE) - if (!pool_metadata$valid) { - pool_warn("Object was destroyed twice.") - return() - } - pool_metadata$valid <- FALSE - private$cancelScheduledTask(object, "validateHandle") - private$cancelScheduledTask(object, "destroyHandle") onDestroy(object) }, error = function(e) { pool_warn(c( From c75bcc4499a58a61008fc92712a90c08adbcc25a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sat, 28 Jan 2023 17:20:35 -0600 Subject: [PATCH 12/12] Correct test name --- tests/testthat/{test-DBI-pool.R => test-hooks.R} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/testthat/{test-DBI-pool.R => test-hooks.R} (100%) diff --git a/tests/testthat/test-DBI-pool.R b/tests/testthat/test-hooks.R similarity index 100% rename from tests/testthat/test-DBI-pool.R rename to tests/testthat/test-hooks.R