From f08af0861142d667038d2c33273c0e71a3c543d6 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 13 Jan 2025 16:37:39 -0500 Subject: [PATCH] Get away from a legacy function name that does not match its implementation --- NAMESPACE | 1 - R/find-port.R | 81 ++++++++++++++--------------- tests/testthat/test-find-port.R | 33 ++++++------ tests/testthat/test-shared-secret.R | 2 +- 4 files changed, 54 insertions(+), 63 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index b72b0fc8..a61630b7 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -116,7 +116,6 @@ importFrom(lifecycle,deprecated) importFrom(magrittr,"%>%") importFrom(rlang,"%||%") importFrom(rlang,missing_arg) -importFrom(stats,runif) importFrom(stats,setNames) importFrom(utils,file.edit) importFrom(utils,installed.packages) diff --git a/R/find-port.R b/R/find-port.R index 4cc3a1de..945f95e8 100644 --- a/R/find-port.R +++ b/R/find-port.R @@ -1,51 +1,33 @@ -# Exclude unsafe ports from Chrome https://src.chromium.org/viewvc/chrome/trunk/src/net/base/net_util.cc?view=markup#l127 -unsafePortList <- c(0, asNamespace("httpuv")[["unsafe_ports"]]) - -#' Get a random port between 3k and 10k, excluding unsafe ports. If a preferred -#' port has already been registered in .globals, use that instead. -#' @importFrom stats runif -#' @noRd -getRandomPort <- function() { - port <- 0 - while (port %in% unsafePortList) { - port <- round(runif(1, 3000, 10000)) - } - port +# Ports that are considered unsafe by Chrome +# http://superuser.com/questions/188058/which-ports-are-considered-unsafe-on-chrome +# https://github.com/rstudio/shiny/issues/1784 +unsafePorts <- function() { + asNamespace("httpuv")[["unsafe_ports"]] } -findRandomPort <- function() { - port <- - if (!is.null(.globals$port)) { - # Start by trying the .globals$port - .globals$port - } else { - getRandomPort() - } - for (i in 1:10) { - tryCatch( - srv <- httpuv::startServer("127.0.0.1", port, list(), quiet = TRUE), - error = function(e) { - port <<- 0 - } - ) - if (port != 0) { - # Stop the temporary server, and retain this port number. - httpuv::stopServer(srv) - .globals$port <- port - break - } - port <- getRandomPort() - } - - if (port == 0) { +randomPort <- function(..., n = 10) { + tryCatch({ + port <- httpuv::randomPort(..., n = n) + }, httpuv_unavailable_port = function(e) { stop( "Unable to start a Plumber server. ", - "We were unable to find a free port in 10 tries." + paste0("We were unable to find a free port in ", n, " tries.") ) - } + }) + return(port) +} - as.integer(port) +portIsAvailable <- function(port) { + tryCatch( + { + randomPort(min = port, max = port) + TRUE + }, + error = function(e) { + FALSE + } + ) } #' Find a port either using the assigned port or randomly search 10 times for an @@ -54,7 +36,20 @@ findRandomPort <- function() { #' @noRd findPort <- function(port = NULL) { if (is.null(port)) { - return(findRandomPort()) + # Try to use the most recently used _random_ port + if ( + (!is.null(.globals$last_random_port)) && + portIsAvailable(.globals$last_random_port) + ) { + return(.globals$last_random_port) + } + + # Find an available port + port <- randomPort() + + # Save the random port for future use + .globals$last_random_port <- port + return(.globals$last_random_port) } port_og <- port @@ -75,7 +70,7 @@ findPort <- function(port = NULL) { stop("Port must be an integer in the range 1024 to 49151 (inclusive).") } - if (port %in% unsafePortList) { + if (port == 0 || port %in% unsafePorts()) { stop("Port ", port, " is an unsafe port. Please choose another port.") } diff --git a/tests/testthat/test-find-port.R b/tests/testthat/test-find-port.R index d99c70dd..49a81023 100644 --- a/tests/testthat/test-find-port.R +++ b/tests/testthat/test-find-port.R @@ -1,24 +1,20 @@ context("find port") test_that("ports can be randomly found", { - foundPorts <- NULL - - for (i in 1:50){ - p <- getRandomPort() - expect_gte(p, 3000) - expect_lte(p, 10000) - - foundPorts <- c(foundPorts, p) - } + foundPorts <- unlist(lapply(1:50, function(i) { + p <- randomPort() + # Use findPort to verify the port's value + findPort(p) + })) # It's possible we got a collision or two, but shouldn't have many. expect_gt(length(unique(foundPorts)), 45) }) test_that("global port used if available", { - .globals$port <- 1234 - expect_equal(findPort(), 1234) - rm("port", envir = .globals) + .globals$last_random_port <- 12345 + expect_equal(findPort(), 12345) + rm("last_random_port", envir = .globals) }) test_that("integer type is returned", { @@ -46,27 +42,28 @@ test_that("finds a good port and persists it", { p <- findPort() # Persisted - expect_equal(.globals$port, p) + expect_equal(.globals$last_random_port, p) # Check that we can actually start a server srv <- httpuv::startServer("127.0.0.1", p, list()) # Cleanup - rm("port", envir = .globals) + rm("last_random_port", envir = .globals) httpuv::stopServer(srv) }) test_that("we don't pin to globals$port if it's occupied", { testthat::skip_on_cran() - srv <- httpuv::startServer("127.0.0.1", 1234, list()) - .globals$port <- 1234 + ex_p <- 12345 + srv <- httpuv::startServer("127.0.0.1", ex_p, list()) + .globals$last_random_port <- ex_p p <- findPort() # It should shuffle to find another port. - expect_true(p != 1234) + expect_true(p != ex_p) - rm("port", envir = .globals) + rm("last_random_port", envir = .globals) httpuv::stopServer(srv) }) diff --git a/tests/testthat/test-shared-secret.R b/tests/testthat/test-shared-secret.R index 6f882f3c..2caa3545 100644 --- a/tests/testthat/test-shared-secret.R +++ b/tests/testthat/test-shared-secret.R @@ -3,7 +3,7 @@ context("shared secret") test_that("requests with shared secrets pass, w/o fail", { options(`plumber.sharedSecret`="abcdefg") - pr <- pr() + pr <- pr() %>% pr_set_debug(FALSE) pr$handle("GET", "/", function(){ 123 }) req <- make_req("GET", "/", pr = pr)