SimonDanisch / Bonito.jl

Serving JS to the browser
MIT License
204 stars 29 forks source link

hard close sessions relative to ready time not creation time #248

Closed bjarthur closed 1 week ago

bjarthur commented 3 weeks ago

fixes https://github.com/SimonDanisch/Bonito.jl/issues/234

please review carefully as i'm still only very superficially familiar with the Bonito codebase.

bjarthur commented 3 weeks ago

not sure what's wrong with the tests. if i manually enter the commands leading up to the error on the REPL, it succeeds and the test passes:

julia> using Bonito, Electron

julia> edisplay = Bonito.use_electron_display(; devtools=true);

julia> app = App() do s
                   return DOM.div()
               end

julia> display(edisplay, app)
Bonito.HTTPServer.ElectronDisplay{Window}(Window(Application(Base.PipeEndpoint(RawFD(26) paused, 0 bytes waiting), Process(setenv(`/Users/arthurb/.julia/artifacts/28d75c9551b709f4f8172bf33e631b433c6c68d6/Julia.app/Contents/MacOS/Julia /Users/arthurb/.julia/packages/Electron/LXyZ0/src/main.js /var/folders/s5/8d629n5d7nsf37f60_91wzr40000gq/T/jlel-283cf0705e4a11ef11cfa765b677554d /var/folders/s5/8d629n5d7nsf37f60_91wzr40000gq/T/jlel-sn-283cf7825e4a11ef3deac15fb6b7565d S0iUwWygzsilJb/IUuZsljEV51PteEn+Q8mDvVoAN5aNXdobgzhxJAZImYr2kjwBkKti8lhggjaopH8jrm7ZkqR1ES1VnKI1Abg1xhNWrKbCwGqL7ISsN7rdI7cNUigpt4AFlnBsxDvd7tFardJgCpKMX161vw0oJhhz1526Iqc=`,["XPC_FLAGS=0x0", "_CE_M=", "PATH=/Users/arthurb/.juliaup/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/X11/bin:/Library/Apple/usr/bin:/opt/homebrew/Caskroom/miniconda/base/condabin:/opt/homebrew/Caskroom/miniconda/base/envs/songexplorer/bin/songexplorer/test:/opt/homebrew/Caskroom/miniconda/base/envs/songexplorer/bin/songexplorer/src:/opt/homebrew/Caskroom/miniconda/base/envs/songexplorer/bin:/Users/arthurb/.cargo/bin", "JULIA_PROJECT=@.", "PWD=/Users/arthurb/.julia/dev/Bonito", "DISPLAY=/private/tmp/com.apple.launchd.yiE995Vw7a/org.xquartz:0", "XPC_SERVICE_NAME=0", "TERM_PROGRAM=Apple_Terminal", "HOMEBREW_PREFIX=/opt/homebrew", "CONDA_PYTHON_EXE=/opt/homebrew/Caskroom/miniconda/base/bin/python"  …  "_CE_CONDA=", "SECURITYSESSIONID=186ac", "USER=arthurb", "JULIA_EDITOR=vi", "CONDA_SHLVL=0", "CONDA_EXE=/opt/homebrew/Caskroom/miniconda/base/bin/conda", "TERM=xterm-256color", "HOME=/Users/arthurb", "TERM_PROGRAM_VERSION=453", "OPENBLAS_MAIN_FREE=1"]), ProcessRunning), [1 window]), 1, true, Channel{Any}(128)), Bonito.HTTPServer.BrowserDisplay(Server:
  isrunning: true
  listen_url: http://localhost:9384
  online_url: http://localhost:9384
  http routes: 2
    /browser-display => Bonito.DisplayHandler
    r"\Q/assets/\E(?:(?:(?:[\da-f]){40})(?:-.*))" => HTTPAssetServer
  websocket routes: 1
    /10be6a03-dbe6-4370-babd-7b9882c23589 => WebSocketConnection
, false, Bonito.DisplayHandler(Session{WebSocketConnection}:
  id: 10be6a03-dbe6-4370-babd-7b9882c23589
  parent: Nothing
children: 1
  connection: open
  isready: true
  asset_server: Bonito.ChildAssetServer
  queued messages: 0
, Server:
  isrunning: true
  listen_url: http://localhost:9384
  online_url: http://localhost:9384
  http routes: 2
    /browser-display => Bonito.DisplayHandler
    r"\Q/assets/\E(?:(?:(?:[\da-f]){40})(?:-.*))" => HTTPAssetServer
  websocket routes: 1
    /10be6a03-dbe6-4370-babd-7b9882c23589 => WebSocketConnection
, "/browser-display", App(Bonito.var"#8#14"{var"#1#2"}(var"#1#2"()), Base.RefValue{Union{Nothing, Session}}(Session{Bonito.SubConnection}:
  id: a0456dd6-a571-465f-bade-c604888c269a
  parent: Session{WebSocketConnection}
  connection: open
  isready: true
  asset_server: Bonito.ChildAssetServer
  queued messages: 0
), "Bonito App", false))))

julia> close(edisplay.window)

julia> server = edisplay.browserdisplay.server
Server:
  isrunning: true
  listen_url: http://localhost:9384
  online_url: http://localhost:9384
  http routes: 2
    /browser-display => Bonito.DisplayHandler
    r"\Q/assets/\E(?:(?:(?:[\da-f]){40})(?:-.*))" => HTTPAssetServer
  websocket routes: 0

julia> isempty(server.websocket_routes.table)
true

julia> success = Bonito.wait_for(() -> isempty(server.websocket_routes.table); timeout=5)
:success

and increasing that timeout to 500 doesn't help with the automatically run tests.

frankier commented 1 week ago

I believe the PR may be revealing a bug somewhere else rather than being buggy itself. Either way it's important to track down. I think e.g. a session is somehow never made ready somewhere in the tests? i.e. i think the problem is related to adding isready(session) rather than adding session.ready_time.

The tests are not really isolated. If you comment everything except for https://github.com/SimonDanisch/Bonito.jl/blob/1964ec02972e5e31c496ad1ac8e20beab81bc26b/test/subsessions.jl#L1-L58 , then the test pass -- so as you say it is not the failing testset and its previous testset causing it. This means it is coming from earlier code, or a combination of earlier code and the previous testset.

SimonDanisch commented 1 week ago

This is not the correct fix for the issue. init_session(session::Session) is only called after a websocket connection has been established, while the close after 20s is meant to close sessions that were never able to open a websocket connection. See: https://github.com/SimonDanisch/Bonito.jl/pull/251