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
Merged

Check that a chromote connection is alive #136

merged 12 commits into from Feb 2, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jan 30, 2024

When your computer goes to sleep, the websocket and all debugging sessions are closed. This PR ensures that Chromote and ChromoteSession report this accurately.

I distinguished between active and alive for Chromote but not ChromoteSession since the browser process continues to run in the background, even though we have lost our connection to it. There's no way to restore a closed debugging session. However, I'm not sure about the names; maybe I have alive and active around the wrong way?

Part of #94


I tested this code with:

library(chromote)

sess <- ChromoteSession$new()
sess$is_active()
#> [1] TRUE
sess$parent$is_alive()
#> [1] TRUE

# SLEEP

sess$is_active()
#> [1] FALSE
sess$parent$is_alive()
#> [1] FALSE

When your computer goes to sleep, the websocket and all debugging sessions are closed. This PR ensures that Chromote and ChromoteSession report this accurately.

I distinguished between active and alive for Chromote but not ChromoteSession since the browser process continues to run in the background, even though we have lost our connection to it. There's no way to restore a closed debugging session. However, I'm not sure about the names; maybe I have alive and active around the wrong way?

Part of #94
@hadley hadley requested a review from wch January 30, 2024 14:09
@hadley
Copy link
Member Author

hadley commented Jan 31, 2024

Now Chromote will reconnect if the websocket is dead but the underlying process is alive:

library(chromote)

chrome <- Chromote$new()
chrome$is_alive()
#> [1] TRUE
chrome$is_active()
#> [1] TRUE
chrome$check_active()

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

chrome$is_alive()
#> [1] TRUE
chrome$is_active()
#> [1] FALSE
chrome$check_active()
#> ! Reconnecting to chrome process.
#> ℹ All active sessions will be need to be respawned.
chrome$is_active()
#> [1] TRUE

chrome$close()
#> [1] FALSE
chrome$is_alive()
#> [1] FALSE
chrome$is_active()
#> [1] FALSE
chrome$check_active()
#> Error in `chrome$check_active()`:
#> ! Chromote has been closed.

R/chromote.R Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented Jan 31, 2024

Overall I'm OK with this change, but we will want to make it very clear to the user what the difference is between is_alive and is_active.

Perhaps the print method for Chromote could also print out that information? Like Process is [not] alive and Websocket connection to Chrome is [not] active.

And we'll also want to mark it as a breaking change in the NEWS file.

@hadley
Copy link
Member Author

hadley commented Feb 1, 2024

I've addressed your concerns, apart from the print method, which I agree is a good idea and I'll do as part of #140, once this PR is merged.

Since your last review, I've added some additional logic to ChromoteSession to distinguish between the session and target being closed. This means that you get a reasonable error message if you $close the session (and hence the underlying target) and then try to $respawn() it. This is just a static value I cache in R, so it'll be incorrect if you close the target some other way, but I think that'll be uncommon and thus not worth handling. Let me know if you disagree.

library(chromote)

sess <- ChromoteSession$new()
sess$is_active()
#> [1] TRUE

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

sess$is_active()
#> [1] FALSE

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.

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

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

@hadley hadley requested a review from wch February 1, 2024 14:03
Comment on lines 545 to 547
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.

@@ -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.

@wch
Copy link
Collaborator

wch commented Feb 2, 2024

I think the overall approach is good -- just have a couple questions.

@wch wch merged commit bbc13af into main Feb 2, 2024
12 checks passed
@wch wch deleted the check-alive branch February 2, 2024 16:40
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.

None yet

2 participants