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

Check that a chromote connection is alive #136

Merged
merged 12 commits into from
Feb 2, 2024
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# chromote (development version)

* `Chromote` gains a new `is_alive()` method equivalent to the old `is_active()` method; i.e. it reports on if there is an active chrome process running in the background.

* Breaking change: `Chromote$is_active()` method now reports if there is an active connection to the underlying chrome instance, rather than whether or not that instance is alive (#94).

* `--disable-gpu` is no longer included in the default Chrome arguments.

* `ChromoteSession` now records the `targetId`. This eliminates one round-trip to the browser when viewing or closing a session. You can now call the `$respawn()` method if a session terminates and you want to reconnect to the same target (#94).
Expand Down
3 changes: 3 additions & 0 deletions R/browser.R
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ Browser <- R6Class("Browser",
#' @description Browser process
get_process = function() private$process,

#' @description Is the process alive?
is_alive = function() private$process$is_alive(),

#' @description Browser Host
get_host = function() private$host,

Expand Down
75 changes: 55 additions & 20 deletions R/chromote.R
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ Chromote <- R6Class(
list2env(self$protocol, self)

private$event_manager <- EventManager$new(self)
private$is_active_ <- TRUE

self$wait_for(p)

Expand Down Expand Up @@ -174,6 +173,7 @@ Chromote <- R6Class(
#' `ChromoteSession` object. Otherwise, block during initialization, and
#' return a `ChromoteSession` object directly.
new_session = function(width = 992, height = 1323, targetId = NULL, wait_ = TRUE) {
self$check_active()
create_session(
chromote = self,
width = width,
Expand Down Expand Up @@ -213,9 +213,7 @@ Chromote <- R6Class(
#' @param sessionId Determines which [`ChromoteSession`] with the
#' corresponding to send the command to.
send_command = function(msg, callback = NULL, error = NULL, timeout = NULL, sessionId = NULL) {
if (!private$is_active_) {
stop("Chromote object is closed.")
}
self$check_active()

private$last_msg_id <- private$last_msg_id + 1
msg$id <- private$last_msg_id
Expand Down Expand Up @@ -320,11 +318,40 @@ Chromote <- R6Class(
paste0("http://", private$browser$get_host(), ":", private$browser$get_port(), path)
},

#' @description Retrieve active status
#' Once initialized, the value returned is `TRUE`. If `$close()` has been
#' called, this value will be `FALSE`.
#' @description
#' Is there an active websocket connection to the browser process?
is_active = function() {
private$is_active_
self$is_alive() && private$ws$readyState() %in% c(0L, 1L)
},

#' @description
#' Is the underlying browser process running?
is_alive = function() {
private$browser$is_alive()
},

#' @description Check that a chromote instance is active and alive.
#' Will automatically reconnect if browser process is alive, but
#' there's no active web socket connection.
check_active = function() {
if (!self$is_alive()) {
stop("Chromote has been closed.")
}

if (!self$is_active()) {
inform(c(
"!" = "Reconnecting to chrome process.",
i = "All active sessions will be need to be respawned.")
)
self$connect()

# Mark all sessions as closed
for (session in private$sessions) {
session$mark_closed(FALSE)
}
private$sessions <- list()
}
invisible(self)
},

#' @description Retrieve [`Browser`]` object
Expand All @@ -335,13 +362,24 @@ Chromote <- R6Class(

#' @description Close the [`Browser`] object
close = function() {
if (private$is_active_) {
self$Browser$close()
private$is_active_ <- FALSE
return(TRUE)
} else {
FALSE
# Must be alive to be active so we cache value before closing process
is_active <- self$is_active()

if (self$is_alive()) {
if (is_active) {
# send a message to the browser requesting that it close
self$Browser$close()
} else {
# terminate the process
private$browser$close()
}
}

if (is_active) {
private$ws$close()
}

invisible()
},

#' @field default_timeout Default timeout in seconds for \pkg{chromote} to
Expand All @@ -354,7 +392,6 @@ Chromote <- R6Class(
private = list(
browser = NULL,
ws = NULL,
is_active_ = NULL,

# =========================================================================
# Browser commands
Expand Down Expand Up @@ -400,9 +437,7 @@ Chromote <- R6Class(
event_manager = NULL,

register_event_listener = function(event, callback = NULL, timeout = NULL) {
if (!private$is_active_) {
stop("Chromote object is closed.")
}
self$check_active()
private$event_manager$register_event_listener(event, callback, timeout)
},

Expand All @@ -416,7 +451,7 @@ Chromote <- R6Class(
return()

private$sessions[[sid]] <- NULL
session$mark_closed()
session$mark_closed(TRUE)
})
},

Expand Down Expand Up @@ -496,7 +531,7 @@ default_chromote_object <- function() {
#' @rdname default_chromote_object
#' @export
has_default_chromote_object <- function() {
!is.null(globals$default_chromote) && globals$default_chromote$is_active()
!is.null(globals$default_chromote) && globals$default_chromote$is_alive()
}

#' @param x A \code{\link{Chromote}} object.
Expand Down
55 changes: 37 additions & 18 deletions R/chromote_session.R
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ ChromoteSession <- R6Class(

private$auto_events <- auto_events
private$event_manager <- EventManager$new(self)
private$is_active_ <- TRUE
private$session_is_active <- TRUE
private$target_is_active <- TRUE

# Find pixelRatio for screenshots
p <- p$
Expand All @@ -117,7 +118,7 @@ ChromoteSession <- R6Class(
warning("Chromote has received a Inspector.targetCrashed event. This means that the ChromoteSession has probably crashed.")
# Even if no targetId nor sessionId is returned by Inspector.targetCashed
# mark the session as closed. This will close all sessions..
self$mark_closed()
self$mark_closed(TRUE)
})
})
}
Expand Down Expand Up @@ -184,6 +185,10 @@ ChromoteSession <- R6Class(
#' when the `ChromoteSession` is closed. Otherwise, block until the
#' `ChromoteSession` has closed.
close = function(wait_ = TRUE) {
if (!private$target_is_active) {
return(invisible())
}

# Even if this session calls Target.closeTarget, the response from
# the browser is sent without a sessionId. In order to wait for the
# correct browser response, we need to invoke this from the parent's
Expand All @@ -192,7 +197,7 @@ ChromoteSession <- R6Class(

p <- p$then(function(value) {
if (isTRUE(value$success)) {
self$mark_closed()
self$mark_closed(TRUE)
}
invisible(value$success)
})
Expand Down Expand Up @@ -429,7 +434,7 @@ ChromoteSession <- R6Class(
#' as this session. This is useful if the session has been closed but the target still
#' exists.
respawn = function() {
if (!private$is_active_) {
if (!private$target_is_active) {
stop("Can't respawn session; target has been closed.")
}

Expand Down Expand Up @@ -507,9 +512,7 @@ ChromoteSession <- R6Class(
#' @param timeout Number of milliseconds for Chrome DevTools Protocol
#' execute a method.
send_command = function(msg, callback = NULL, error = NULL, timeout = NULL) {
if (!private$is_active_) {
stop("Session ", private$session_id, " is closed.")
}
self$check_active()
self$parent$send_command(msg, callback, error, timeout, sessionId = private$session_id)
},

Expand All @@ -535,19 +538,36 @@ ChromoteSession <- R6Class(
private$event_manager$invoke_event_callbacks(event, params)
},

#' @description
#' Disable callbacks for a given session.
#'
#' For internal use only.
mark_closed = function() {
private$is_active_ <- FALSE
#' @description Mark a session, and optionally, the underlying target,
#' as closed. For internal use only.
#' @param target_closed Has the underlying target been closed as well as the
#' active debugging session?
mark_closed = function(target_closed) {
private$session_is_active <- FALSE
private$target_is_active <- FALSE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like target_closed is unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. I realised that I forgot to call spawn() before closing in my script test. Here's the new output:

library(chromote)

sess <- ChromoteSession$new()
sess$check_active()
#> NULL

# SLEEP -----------------------

sess$check_active()
#> Error in `sess$check_active()`:
#> ! Session has been closed.
#> ℹ Call session$respawn() to create a new session that connects to the same target.

sess2 <- sess$respawn()
#> ! Reconnecting to chrome process.
#> ℹ All active sessions will be need to be respawned.

sess$close()

sess$respawn()
#> Error in `sess$respawn()`:
#> ! Can't respawn session; target has been closed.

},

#' @description Retrieve active status
#' Once initialized, the value returned is `TRUE`. If `$close()` has been
#' called, this value will be `FALSE`.
is_active = function() {
private$is_active_
private$session_is_active && private$target_is_active && self$parent$is_active()
},

#' @description Check that a session is active, erroring if not.
check_active = function() {
if (self$is_active()) {
return()
}

if (private$target_is_active) {
abort(c(
"Session has been closed.",
i = "Call session$respawn() to create a new session that connects to the same target."
))
} else {
abort("Session and underlying target have been closed.")
}
},

#' @description Initial promise
Expand All @@ -569,16 +589,15 @@ ChromoteSession <- R6Class(
private = list(
session_id = NULL,
target_id = NULL,
is_active_ = NULL,
session_is_active = NULL,
target_is_active = NULL,
Copy link
Collaborator

@wch wch Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about target_is_alive? (I'm asking honestly, not suggesting that it should or should not be that name.) I ask because it might help conceptually separate the two alive names. Also the target exists independently of any connection to it, sort of like how the Chrome process exists independently of a connection to it, and in that case we call it alive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind changing the name, but calling it alive does introduce another inconsistency — Chromote$is_alive() is always accurate because we have a connection to the process that we can check; but there's currently no analog for the target. I could add an is_alive() method that checks if the target ID is found in the chromote instance, but I'm not sure if that's worth it (plus calling that repeatedly might be expensive).

So while I agree that "active" isn't the best name, and has some potential for confusion, I think changing it to "alive" is a net neutral because it just introduces a different source confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's keep it as "active" then.

event_manager = NULL,
pixel_ratio = NULL,
auto_events = NULL,
init_promise_ = NULL,

register_event_listener = function(event, callback = NULL, timeout = NULL) {
if (!private$is_active_) {
stop("Session ", private$session_id, " is closed.")
}
self$check_active()
private$event_manager$register_event_listener(event, callback, timeout)
}
)
Expand Down
11 changes: 11 additions & 0 deletions man/Browser.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion man/Chrome.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion man/ChromeRemote.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 25 additions & 3 deletions man/Chromote.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading