From ab5275d7a21a9e4c2988f71163b237e285d08304 Mon Sep 17 00:00:00 2001 From: "dyfan.jones" Date: Mon, 26 Jun 2023 18:50:03 +0100 Subject: [PATCH 1/4] split timeout and connect_timeout (#610) --- paws.common/NEWS.md | 1 + paws.common/R/client.R | 2 +- paws.common/R/net.R | 15 +++++++++++---- paws.common/R/request.R | 2 +- paws.common/tests/testthat/test_net.R | 17 +++++++++++++++-- 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/paws.common/NEWS.md b/paws.common/NEWS.md index 0a03effceb..e8af183f16 100644 --- a/paws.common/NEWS.md +++ b/paws.common/NEWS.md @@ -1,5 +1,6 @@ # paws.common 0.5.8 * fix mismatch apparent method as.list.struct (#634) +* split timeout and connecttimeout in http call (#610) # paws.common 0.5.7 * skip network unit test on cran (#632) diff --git a/paws.common/R/client.R b/paws.common/R/client.R index 58a6a0e0ab..df2427adea 100644 --- a/paws.common/R/client.R +++ b/paws.common/R/client.R @@ -13,7 +13,7 @@ Config <- struct( disable_ssl = FALSE, close_connection = FALSE, max_retries = -1, - timeout = 60, + connect_timeout = 60, retryer = NULL, disable_param_validation = FALSE, disable_compute_checksums = FALSE, diff --git a/paws.common/R/net.R b/paws.common/R/net.R index 6549ee28cf..802acc2dcc 100644 --- a/paws.common/R/net.R +++ b/paws.common/R/net.R @@ -23,6 +23,7 @@ HttpRequest <- struct( request_uri = "", tls = NULL, cancel = NULL, + connect_timeout = NULL, timeout = NULL, response = NULL, ctx = list(), @@ -53,10 +54,11 @@ HttpResponse <- struct( # @param url The URL to send the request to. # @param body The body to send in the request, in bytes. # @param close Whether to immediately close the connection, or else reuse connections. -# @param timeout How long to wait for an initial response. +# @param connect_timeout How long to wait for an initial response. +# @param timeout Timeout for the entire request. # @param dest Control where the response body is written # @param header list of HTTP headers to add to the request -new_http_request <- function(method, url, body = NULL, close = FALSE, timeout = NULL, dest = NULL, header = list()) { +new_http_request <- function(method, url, body = NULL, close = FALSE, connect_timeout = NULL, timeout = NULL, dest = NULL, header = list()) { if (method == "") { method <- "GET" } @@ -74,6 +76,7 @@ new_http_request <- function(method, url, body = NULL, close = FALSE, timeout = body = body, host = u$host, close = close, + connect_timeout = connect_timeout, timeout = timeout, dest = dest ) @@ -104,8 +107,12 @@ issue <- function(http_request) { headers["Connection"] <- "close" } body <- http_request$body - timeout <- httr::config(connecttimeout = http_request$timeout) - if (is.null(http_request$timeout)) timeout <- NULL + + timeout_config <- Filter( + Negate(is.null), + list(connecttimeout = http_request$connect_timeout, timeout = http_request$timeout) + ) + timeout <- do.call(httr::config, timeout_config) if (url == "") { stop("no url provided") diff --git a/paws.common/R/request.R b/paws.common/R/request.R index 791d765ffe..fdd2665b32 100644 --- a/paws.common/R/request.R +++ b/paws.common/R/request.R @@ -114,7 +114,7 @@ new_request <- function(client, operation, params, data, dest = NULL) { url = "", body = NULL, close = client$config$close_connection, - timeout = client$config$timeout, + connect_timeout = client$config$connect_timeout, dest = dest ) diff --git a/paws.common/tests/testthat/test_net.R b/paws.common/tests/testthat/test_net.R index e756b264b6..412ca2073a 100644 --- a/paws.common/tests/testthat/test_net.R +++ b/paws.common/tests/testthat/test_net.R @@ -16,6 +16,19 @@ test_that("issue", { } }) +test_that("connect_timeout", { + req <- HttpRequest( + method = "GET", + url = parse_url("https://example.com:81"), + connect_timeout = 1 + ) + quietly <- function(expr) suppressMessages(tryCatch(expr, error = function(e) {})) + time <- system.time({ + quietly(issue(req)) + }) + expect_equivalent(time["elapsed"], 1, tolerance = 0.5) +}) + test_that("timeout", { req <- HttpRequest( method = "GET", @@ -29,14 +42,14 @@ test_that("timeout", { expect_equivalent(time["elapsed"], 1, tolerance = 0.5) }) -test_that("timeout does not affect established connections", { +test_that("connect_timeout does not affect established connections", { # Avoid CRAN check errors due to unavailable network resources. skip_on_cran() req <- HttpRequest( method = "GET", url = parse_url("https://httpbin.org/delay/3"), - timeout = 1 + connect_timeout = 1 ) resp <- issue(req) From 4d139abb3ccdc99d5e51caa64476d502469626dc Mon Sep 17 00:00:00 2001 From: "dyfan.jones" Date: Mon, 26 Jun 2023 19:02:39 +0100 Subject: [PATCH 2/4] switch to timeout_ms --- paws.common/R/config.R | 6 +++--- paws.common/R/net.R | 10 +++++----- paws.common/tests/testthat/test_net.R | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/paws.common/R/config.R b/paws.common/R/config.R index 77943afd0a..879c06c8d5 100644 --- a/paws.common/R/config.R +++ b/paws.common/R/config.R @@ -179,7 +179,7 @@ get_instance_metadata <- function(query_path = "") { metadata_token_request <- new_http_request( "PUT", metadata_token_url, - timeout = 1, + timeout_ms = 1000, header=c("X-aws-ec2-metadata-token-ttl-seconds"= token_ttl) ) @@ -204,11 +204,11 @@ get_instance_metadata <- function(query_path = "") { metadata_request <- new_http_request( "GET", metadata_url, - timeout = 1, + timeout_ms = 1000, header = c("X-aws-ec2-metadata-token"= token) ) } else { - metadata_request <- new_http_request("GET", metadata_url, timeout = 1) + metadata_request <- new_http_request("GET", metadata_url, timeout_ms = 1000) } metadata_response <- tryCatch( { diff --git a/paws.common/R/net.R b/paws.common/R/net.R index 802acc2dcc..49b9c941f3 100644 --- a/paws.common/R/net.R +++ b/paws.common/R/net.R @@ -24,7 +24,7 @@ HttpRequest <- struct( tls = NULL, cancel = NULL, connect_timeout = NULL, - timeout = NULL, + timeout_ms = NULL, response = NULL, ctx = list(), dest = NULL @@ -55,10 +55,10 @@ HttpResponse <- struct( # @param body The body to send in the request, in bytes. # @param close Whether to immediately close the connection, or else reuse connections. # @param connect_timeout How long to wait for an initial response. -# @param timeout Timeout for the entire request. +# @param timeout_ms Timeout for the entire request. # @param dest Control where the response body is written # @param header list of HTTP headers to add to the request -new_http_request <- function(method, url, body = NULL, close = FALSE, connect_timeout = NULL, timeout = NULL, dest = NULL, header = list()) { +new_http_request <- function(method, url, body = NULL, close = FALSE, connect_timeout = NULL, timeout_ms = NULL, dest = NULL, header = list()) { if (method == "") { method <- "GET" } @@ -77,7 +77,7 @@ new_http_request <- function(method, url, body = NULL, close = FALSE, connect_ti host = u$host, close = close, connect_timeout = connect_timeout, - timeout = timeout, + timeout_ms = timeout_ms, dest = dest ) return(req) @@ -110,7 +110,7 @@ issue <- function(http_request) { timeout_config <- Filter( Negate(is.null), - list(connecttimeout = http_request$connect_timeout, timeout = http_request$timeout) + list(connecttimeout = http_request$connect_timeout, timeout_ms = http_request$timeout_ms) ) timeout <- do.call(httr::config, timeout_config) diff --git a/paws.common/tests/testthat/test_net.R b/paws.common/tests/testthat/test_net.R index 412ca2073a..95d48e4ea2 100644 --- a/paws.common/tests/testthat/test_net.R +++ b/paws.common/tests/testthat/test_net.R @@ -29,11 +29,11 @@ test_that("connect_timeout", { expect_equivalent(time["elapsed"], 1, tolerance = 0.5) }) -test_that("timeout", { +test_that("timeout_ms", { req <- HttpRequest( method = "GET", url = parse_url("https://example.com:81"), - timeout = 1 + timeout_ms = 1000 ) quietly <- function(expr) suppressMessages(tryCatch(expr, error = function(e) {})) time <- system.time({ From f03f4c18a95379cc08cc284afdbf55a9deadfb87 Mon Sep 17 00:00:00 2001 From: "dyfan.jones" Date: Mon, 26 Jun 2023 19:07:52 +0100 Subject: [PATCH 3/4] revert to timeout --- paws.common/R/config.R | 6 +++--- paws.common/R/net.R | 10 +++++----- paws.common/tests/testthat/test_net.R | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/paws.common/R/config.R b/paws.common/R/config.R index 879c06c8d5..77943afd0a 100644 --- a/paws.common/R/config.R +++ b/paws.common/R/config.R @@ -179,7 +179,7 @@ get_instance_metadata <- function(query_path = "") { metadata_token_request <- new_http_request( "PUT", metadata_token_url, - timeout_ms = 1000, + timeout = 1, header=c("X-aws-ec2-metadata-token-ttl-seconds"= token_ttl) ) @@ -204,11 +204,11 @@ get_instance_metadata <- function(query_path = "") { metadata_request <- new_http_request( "GET", metadata_url, - timeout_ms = 1000, + timeout = 1, header = c("X-aws-ec2-metadata-token"= token) ) } else { - metadata_request <- new_http_request("GET", metadata_url, timeout_ms = 1000) + metadata_request <- new_http_request("GET", metadata_url, timeout = 1) } metadata_response <- tryCatch( { diff --git a/paws.common/R/net.R b/paws.common/R/net.R index 49b9c941f3..802acc2dcc 100644 --- a/paws.common/R/net.R +++ b/paws.common/R/net.R @@ -24,7 +24,7 @@ HttpRequest <- struct( tls = NULL, cancel = NULL, connect_timeout = NULL, - timeout_ms = NULL, + timeout = NULL, response = NULL, ctx = list(), dest = NULL @@ -55,10 +55,10 @@ HttpResponse <- struct( # @param body The body to send in the request, in bytes. # @param close Whether to immediately close the connection, or else reuse connections. # @param connect_timeout How long to wait for an initial response. -# @param timeout_ms Timeout for the entire request. +# @param timeout Timeout for the entire request. # @param dest Control where the response body is written # @param header list of HTTP headers to add to the request -new_http_request <- function(method, url, body = NULL, close = FALSE, connect_timeout = NULL, timeout_ms = NULL, dest = NULL, header = list()) { +new_http_request <- function(method, url, body = NULL, close = FALSE, connect_timeout = NULL, timeout = NULL, dest = NULL, header = list()) { if (method == "") { method <- "GET" } @@ -77,7 +77,7 @@ new_http_request <- function(method, url, body = NULL, close = FALSE, connect_ti host = u$host, close = close, connect_timeout = connect_timeout, - timeout_ms = timeout_ms, + timeout = timeout, dest = dest ) return(req) @@ -110,7 +110,7 @@ issue <- function(http_request) { timeout_config <- Filter( Negate(is.null), - list(connecttimeout = http_request$connect_timeout, timeout_ms = http_request$timeout_ms) + list(connecttimeout = http_request$connect_timeout, timeout = http_request$timeout) ) timeout <- do.call(httr::config, timeout_config) diff --git a/paws.common/tests/testthat/test_net.R b/paws.common/tests/testthat/test_net.R index 95d48e4ea2..412ca2073a 100644 --- a/paws.common/tests/testthat/test_net.R +++ b/paws.common/tests/testthat/test_net.R @@ -29,11 +29,11 @@ test_that("connect_timeout", { expect_equivalent(time["elapsed"], 1, tolerance = 0.5) }) -test_that("timeout_ms", { +test_that("timeout", { req <- HttpRequest( method = "GET", url = parse_url("https://example.com:81"), - timeout_ms = 1000 + timeout = 1 ) quietly <- function(expr) suppressMessages(tryCatch(expr, error = function(e) {})) time <- system.time({ From 37c52b6dc057dbb9edc0e5fbd2a20b17ad6c7239 Mon Sep 17 00:00:00 2001 From: "dyfan.jones" Date: Mon, 26 Jun 2023 19:10:42 +0100 Subject: [PATCH 4/4] add thanks --- paws.common/NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paws.common/NEWS.md b/paws.common/NEWS.md index e8af183f16..f3a5aaf41c 100644 --- a/paws.common/NEWS.md +++ b/paws.common/NEWS.md @@ -1,6 +1,6 @@ # paws.common 0.5.8 * fix mismatch apparent method as.list.struct (#634) -* split timeout and connecttimeout in http call (#610) +* split timeout and connecttimeout in http call (#610). Thanks to @stuart-storypark for identifying issue, and @joakibo for extra insight and testing. # paws.common 0.5.7 * skip network unit test on cran (#632)