-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
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
#Conflicts: # R/chromote.R # R/chromote_session.R # man/ChromoteSession.Rd
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. |
Overall I'm OK with this change, but we will want to make it very clear to the user what the difference is between Perhaps the And we'll also want to mark it as a breaking change in the NEWS file. |
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 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 |
R/chromote_session.R
Outdated
mark_closed = function(target_closed) { | ||
private$session_is_active <- FALSE | ||
private$target_is_active <- FALSE |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I think the overall approach is good -- just have a couple questions. |
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: