Add module for managing multiple running sessions #3
Conversation
| Supervisor.terminate_child(LiveBook.Supervisor, LiveBook.SessionSupervisor) | ||
| Supervisor.restart_child(LiveBook.Supervisor, LiveBook.SessionSupervisor) | ||
| end) | ||
| end |
There was a problem hiding this comment.
Is there any simpler way of cleaning the Supervisor children (and consequently the :global entries), given it's a part of the application supervision tree?
| Phoenix.PubSub.subscribe(LiveBook.PubSub, "sessions") | ||
| end | ||
|
|
||
| session_ids = LiveBook.SessionSupervisor.get_session_ids() |
There was a problem hiding this comment.
Eventually we will be getting more session/notebook data by querying each session process, but I just used ids for now to have some UI.
lib/live_book/session_supervisor.ex
Outdated
| """ | ||
| @spec create_session() :: {:ok, Session.session_id()} | {:error, any()} | ||
| def create_session() do | ||
| id = UUID.uuid4() |
There was a problem hiding this comment.
This is fine for now but in the future the session id will be something based on the filename.
There was a problem hiding this comment.
Btw, I wouldn't use an external dep here either. This is likely good enough for random tokens:
:crypto.strong_rand_bytes(20) |> Base.encode32(case: :lower)
There was a problem hiding this comment.
This is fine for now but in the future the session id will be something based on the filename.
The filename can change while the session is running (or perhaps not be specified at all) and will be just stored in session state. Also people could start multiple sessions by importing the same file. I introduced the id primarily for registering the processes with :global, anything I am missing here?
There was a problem hiding this comment.
The thing is that sessions can be persisted to disk and when we do so, there is definitely a filename. However, if you want to keep a session in memory only, we would need random session IDs. Sounds good to me, we can revisit this once we have persistence and filenames come more into play.
| # Start a fresh SessionSupervisor for each test | ||
| Supervisor.terminate_child(LiveBook.Supervisor, LiveBook.SessionSupervisor) | ||
| Supervisor.restart_child(LiveBook.Supervisor, LiveBook.SessionSupervisor) | ||
| end) |
There was a problem hiding this comment.
I generally prefer to write the tests in a way that it doesn't require the session to be restarted. Luckily you already wrote most tests like this, except for the DynamicSupervisor.which_children(SessionSupervisor) call below, which you can replace by this:
assert List.keymember?(DynamicSupervisor.which_children(SessionSupervisor), pid, 1)
:)
There was a problem hiding this comment.
Yeah, although create_session returns id rather than pid, so I matched against which_children to get the pid and check if it's correctly registered in the :global module. I tried another approach in this commit by introducing get_session_pid method, which enabled us to still test the same behavior without matching on children. What do you think?
jonatanklosko
force-pushed
the
sessions
branch
from
c184f6d
to
ff77f05
Compare
Adds
SessionSupervisor- a dynamic supervisor that allows for spawning multiple notebook session processes. Each process is assigned a random id and is registered using the:globalmodule.I also added a basic UI with updates via
PubSub.sessions.mp4