From 264d6c3ff2c62696f795748f8395da64cae4014c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 2 Dec 2021 16:45:00 +0100 Subject: [PATCH] Add support for controls output type (#710) * Add support for controls output type * Split controls into individual widgets * Adjust ids * Improve widget and controls garbage collection * Allow arbitrary functions as object release hook * Add type to button and input events * Add keyboard status event * Change release hooks into monitor messages * Rename pointer to reference and return an error on bad monitor --- assets/js/app.js | 2 + assets/js/keyboard_control/index.js | 91 +++++++++++ assets/js/lib/utils.js | 7 + assets/js/session/index.js | 10 +- lib/livebook/evaluator.ex | 89 +++-------- lib/livebook/evaluator/io_proxy.ex | 48 ++++-- lib/livebook/evaluator/object_tracker.ex | 150 ++++++++++++++++++ lib/livebook/runtime/erl_dist.ex | 1 + .../runtime/erl_dist/evaluator_supervisor.ex | 6 +- .../runtime/erl_dist/runtime_server.ex | 11 +- lib/livebook/session.ex | 2 +- lib/livebook_web/live/output.ex | 4 + .../live/output/control_component.ex | 81 ++++++++++ ...frame_dynamic.ex => frame_dynamic_live.ex} | 4 +- .../live/output/input_component.ex | 38 ++++- .../live/output/table_dynamic_live.ex | 4 +- .../live/output/vega_lite_dynamic_live.ex | 4 +- test/livebook/evaluator/io_proxy_test.exs | 19 +-- .../evaluator/object_tracker_test.exs | 57 +++++++ test/livebook/evaluator_test.exs | 84 +++++++--- test/livebook_web/live/session_live_test.exs | 19 ++- 21 files changed, 579 insertions(+), 152 deletions(-) create mode 100644 assets/js/keyboard_control/index.js create mode 100644 lib/livebook/evaluator/object_tracker.ex create mode 100644 lib/livebook_web/live/output/control_component.ex rename lib/livebook_web/live/output/{frame_dynamic.ex => frame_dynamic_live.ex} (92%) create mode 100644 test/livebook/evaluator/object_tracker_test.exs diff --git a/assets/js/app.js b/assets/js/app.js index 1552117dd..cf415f553 100644 --- a/assets/js/app.js +++ b/assets/js/app.js @@ -24,6 +24,7 @@ import MarkdownRenderer from "./markdown_renderer"; import Highlight from "./highlight"; import DragAndDrop from "./drag_and_drop"; import PasswordToggle from "./password_toggle"; +import KeyboardControl from "./keyboard_control"; import morphdomCallbacks from "./morphdom_callbacks"; import { loadUserData } from "./lib/user"; @@ -41,6 +42,7 @@ const hooks = { Highlight, DragAndDrop, PasswordToggle, + KeyboardControl, }; const csrfToken = document diff --git a/assets/js/keyboard_control/index.js b/assets/js/keyboard_control/index.js new file mode 100644 index 000000000..4ab986674 --- /dev/null +++ b/assets/js/keyboard_control/index.js @@ -0,0 +1,91 @@ +import { getAttributeOrThrow, parseBoolean } from "../lib/attribute"; +import { cancelEvent } from "../lib/utils"; + +/** + * A hook for ControlComponent to handle user keyboard interactions. + * + * Configuration: + * + * * `data-keydown-enabled` - whether keydown events should be intercepted + * + * * `data-keyup-enabled` - whether keyup events should be intercepted + * + * * `data-target` - the target to send live events to + */ +const KeyboardControl = { + mounted() { + this.props = getProps(this); + + this.handleDocumentKeyDown = (event) => { + handleDocumentKeyDown(this, event); + }; + + // We intentionally register on window rather than document, + // to intercept clicks as early on as possible, even before + // the session shortcuts + window.addEventListener("keydown", this.handleDocumentKeyDown, true); + + this.handleDocumentKeyUp = (event) => { + handleDocumentKeyUp(this, event); + }; + + window.addEventListener("keyup", this.handleDocumentKeyUp, true); + }, + + updated() { + this.props = getProps(this); + }, + + destroyed() { + window.removeEventListener("keydown", this.handleDocumentKeyDown, true); + window.removeEventListener("keyup", this.handleDocumentKeyUp, true); + }, +}; + +function getProps(hook) { + return { + isKeydownEnabled: getAttributeOrThrow( + hook.el, + "data-keydown-enabled", + parseBoolean + ), + isKeyupEnabled: getAttributeOrThrow( + hook.el, + "data-keyup-enabled", + parseBoolean + ), + target: getAttributeOrThrow(hook.el, "data-target"), + }; +} + +function handleDocumentKeyDown(hook, event) { + if (keyboardEnabled(hook)) { + cancelEvent(event); + } + + if (hook.props.isKeydownEnabled) { + if (event.repeat) { + return; + } + + const key = event.key; + hook.pushEventTo(hook.props.target, "keydown", { key }); + } +} + +function handleDocumentKeyUp(hook, event) { + if (keyboardEnabled(hook)) { + cancelEvent(event); + } + + if (hook.props.isKeyupEnabled) { + const key = event.key; + hook.pushEventTo(hook.props.target, "keyup", { key }); + } +} + +function keyboardEnabled(hook) { + return hook.props.isKeydownEnabled || hook.props.isKeyupEnabled; +} + +export default KeyboardControl; diff --git a/assets/js/lib/utils.js b/assets/js/lib/utils.js index 941faa2d0..03acf834b 100644 --- a/assets/js/lib/utils.js +++ b/assets/js/lib/utils.js @@ -127,3 +127,10 @@ export function findChildOrThrow(element, selector) { return child; } + +export function cancelEvent(event) { + // Cancel any default browser behavior. + event.preventDefault(); + // Stop event propagation (e.g. so it doesn't reach the editor). + event.stopPropagation(); +} diff --git a/assets/js/session/index.js b/assets/js/session/index.js index 23c7e36d5..91115893b 100644 --- a/assets/js/session/index.js +++ b/assets/js/session/index.js @@ -5,6 +5,7 @@ import { selectElementContent, smoothlyScrollToElement, setFavicon, + cancelEvent, } from "../lib/utils"; import { getAttributeOrDefault } from "../lib/attribute"; import KeyBuffer from "./key_buffer"; @@ -572,7 +573,7 @@ function initializeFocus(hook) { const element = document.getElementById(htmlId); if (element) { - const focusableEl = elementelement.closest("[data-focusable-id]"); + const focusableEl = element.closest("[data-focusable-id]"); if (focusableEl) { setFocusedEl(hook, focusableEl.dataset.focusableId); @@ -1067,11 +1068,4 @@ function getRuntimeInfoToggle() { return document.querySelector(`[data-element="runtime-info-toggle"]`); } -function cancelEvent(event) { - // Cancel any default browser behavior. - event.preventDefault(); - // Stop event propagation (e.g. so it doesn't reach the editor). - event.stopPropagation(); -} - export default Session; diff --git a/lib/livebook/evaluator.ex b/lib/livebook/evaluator.ex index 6026a1d0b..c3803f268 100644 --- a/lib/livebook/evaluator.ex +++ b/lib/livebook/evaluator.ex @@ -23,14 +23,12 @@ defmodule Livebook.Evaluator do @type t :: %{pid: pid(), ref: reference()} @type state :: %{ + ref: reference(), formatter: module(), io_proxy: pid(), + object_tracker: pid(), contexts: %{ref() => context()}, - initial_context: context(), - # We track the widgets rendered by every evaluation, - # so that we can kill those no longer needed - widget_pids: %{ref() => MapSet.t(pid())}, - widget_counts: %{pid() => non_neg_integer()} + initial_context: context() } @typedoc """ @@ -57,6 +55,8 @@ defmodule Livebook.Evaluator do Options: + * `object_tracker` - a PID of `Livebook.Evaluator.ObjectTracker`, required + * `formatter` - a module implementing the `Livebook.Evaluator.Formatter` behaviour, used for transforming evaluation response before it's sent to the client """ @@ -171,16 +171,18 @@ defmodule Livebook.Evaluator do end def init(opts) do + object_tracker = Keyword.fetch!(opts, :object_tracker) formatter = Keyword.get(opts, :formatter, Evaluator.IdentityFormatter) - {:ok, io_proxy} = Evaluator.IOProxy.start_link() + {:ok, io_proxy} = Evaluator.IOProxy.start_link(self(), object_tracker) - # Use the dedicated IO device as the group leader, - # so that it handles all :stdio operations. + # Use the dedicated IO device as the group leader, so that + # intercepts all :stdio requests and also handles Livebook + # specific ones Process.group_leader(self(), io_proxy) evaluator_ref = make_ref() - state = initial_state(evaluator_ref, formatter, io_proxy) + state = initial_state(evaluator_ref, formatter, io_proxy, object_tracker) evaluator = %{pid: self(), ref: evaluator_ref} :proc_lib.init_ack(evaluator) @@ -188,15 +190,14 @@ defmodule Livebook.Evaluator do loop(state) end - defp initial_state(evaluator_ref, formatter, io_proxy) do + defp initial_state(evaluator_ref, formatter, io_proxy, object_tracker) do %{ evaluator_ref: evaluator_ref, formatter: formatter, io_proxy: io_proxy, + object_tracker: object_tracker, contexts: %{}, - initial_context: initial_context(), - widget_pids: %{}, - widget_counts: %{} + initial_context: initial_context() } end @@ -221,6 +222,8 @@ defmodule Livebook.Evaluator do defp handle_cast({:evaluate_code, send_to, code, ref, prev_ref, opts}, state) do Evaluator.IOProxy.configure(state.io_proxy, send_to, ref) + Evaluator.ObjectTracker.remove_reference(state.object_tracker, {self(), ref}) + context = get_context(state, prev_ref) file = Keyword.get(opts, :file, "nofile") context = put_in(context.env.file, file) @@ -249,18 +252,12 @@ defmodule Livebook.Evaluator do metadata = %{evaluation_time_ms: evaluation_time_ms} send(send_to, {:evaluation_response, ref, output, metadata}) - widget_pids = Evaluator.IOProxy.flush_widgets(state.io_proxy) - state = track_evaluation_widgets(state, ref, widget_pids, output) - {:noreply, state} end defp handle_cast({:forget_evaluation, ref}, state) do - state = - state - |> Map.update!(:contexts, &Map.delete(&1, ref)) - |> garbage_collect_widgets(ref, []) - + state = Map.update!(state, :contexts, &Map.delete(&1, ref)) + Evaluator.ObjectTracker.remove_reference(state.object_tracker, {self(), ref}) {:noreply, state} end @@ -372,56 +369,6 @@ defmodule Livebook.Evaluator do defp internal_dictionary_key?("$" <> _), do: true defp internal_dictionary_key?(_), do: false - # Widgets - - defp track_evaluation_widgets(state, ref, widget_pids, output) do - widget_pids = - case widget_pid_from_output(output) do - {:ok, pid} -> MapSet.put(widget_pids, pid) - :error -> widget_pids - end - - garbage_collect_widgets(state, ref, widget_pids) - end - - defp garbage_collect_widgets(state, ref, widget_pids) do - prev_widget_pids = state.widget_pids[ref] || [] - - state = put_in(state.widget_pids[ref], widget_pids) - - update_in(state.widget_counts, fn counts -> - counts = - Enum.reduce(prev_widget_pids, counts, fn pid, counts -> - Map.update!(counts, pid, &(&1 - 1)) - end) - - counts = - Enum.reduce(widget_pids, counts, fn pid, counts -> - Map.update(counts, pid, 1, &(&1 + 1)) - end) - - {to_remove, to_keep} = Enum.split_with(counts, fn {_pid, count} -> count == 0 end) - - for {pid, 0} <- to_remove do - Process.exit(pid, :shutdown) - end - - Map.new(to_keep) - end) - end - - @doc """ - Checks the given output value for widget pid to track. - """ - @spec widget_pid_from_output(term()) :: {:ok, pid()} | :error - def widget_pid_from_output(output) - - def widget_pid_from_output({_type, pid}) when is_pid(pid) do - {:ok, pid} - end - - def widget_pid_from_output(_output), do: :error - defp get_execution_time_delta(started_at) do System.monotonic_time() |> Kernel.-(started_at) diff --git a/lib/livebook/evaluator/io_proxy.ex b/lib/livebook/evaluator/io_proxy.ex index f50d19c38..c34fe76a4 100644 --- a/lib/livebook/evaluator/io_proxy.ex +++ b/lib/livebook/evaluator/io_proxy.ex @@ -24,9 +24,9 @@ defmodule Livebook.Evaluator.IOProxy do Make sure to use `configure/3` to actually proxy the requests. """ - @spec start_link() :: GenServer.on_start() - def start_link(opts \\ []) do - GenServer.start_link(__MODULE__, opts) + @spec start_link(pid(), pid()) :: GenServer.on_start() + def start_link(evaluator, object_tracker) do + GenServer.start_link(__MODULE__, evaluator: evaluator, object_tracker: object_tracker) end @doc """ @@ -80,7 +80,10 @@ defmodule Livebook.Evaluator.IOProxy do ## Callbacks @impl true - def init(_opts) do + def init(opts) do + evaluator = Keyword.fetch!(opts, :evaluator) + object_tracker = Keyword.fetch!(opts, :object_tracker) + {:ok, %{ encoding: :unicode, @@ -88,8 +91,9 @@ defmodule Livebook.Evaluator.IOProxy do ref: nil, buffer: [], input_cache: %{}, - widget_pids: MapSet.new(), - token_count: 0 + token_count: 0, + evaluator: evaluator, + object_tracker: object_tracker }} end @@ -107,10 +111,6 @@ defmodule Livebook.Evaluator.IOProxy do {:reply, :ok, flush_buffer(state)} end - def handle_call(:flush_widgets, _from, state) do - {:reply, state.widget_pids, %{state | widget_pids: MapSet.new()}} - end - @impl true def handle_info({:io_request, from, reply_as, req}, state) do {reply, state} = io_request(req, state) @@ -198,13 +198,6 @@ defmodule Livebook.Evaluator.IOProxy do defp io_request({:livebook_put_output, output}, state) do state = flush_buffer(state) send(state.target, {:evaluation_output, state.ref, output}) - - state = - case Evaluator.widget_pid_from_output(output) do - {:ok, pid} -> update_in(state.widget_pids, &MapSet.put(&1, pid)) - :error -> state - end - {:ok, state} end @@ -224,6 +217,27 @@ defmodule Livebook.Evaluator.IOProxy do {token, state} end + defp io_request({:livebook_reference_object, object, pid}, state) do + # When the request comes from evaluator we want the pointer + # specific to the current evaluation. For any other process + # we only care about monitoring. + + reference = + if pid == state.evaluator do + {pid, state.ref} + else + {pid, :process} + end + + Evaluator.ObjectTracker.add_reference(state.object_tracker, object, reference) + {:ok, state} + end + + defp io_request({:livebook_monitor_object, object, destination, payload}, state) do + reply = Evaluator.ObjectTracker.monitor(state.object_tracker, object, destination, payload) + {reply, state} + end + defp io_request(_, state) do {{:error, :request}, state} end diff --git a/lib/livebook/evaluator/object_tracker.ex b/lib/livebook/evaluator/object_tracker.ex new file mode 100644 index 000000000..f9588a834 --- /dev/null +++ b/lib/livebook/evaluator/object_tracker.ex @@ -0,0 +1,150 @@ +defmodule Livebook.Evaluator.ObjectTracker do + @moduledoc false + + # This module is an abstraction for tracking objects, + # references to them and garbage collection. + # + # Every object is identified by an arbitrary unique term. + # Processes can reference those objects by adding a pair + # of `{pid, scope}`, scope is an optional additinal term + # distinguishing the reference. + # + # Each reference can be released either manually by calling + # `remove_reference/2` or automatically when the pointing + # process terminates. + # + # When all references for the given object are removed, + # all messages scheduled with `monitor/3` are sent. + + use GenServer + + @type state :: %{ + objects: %{ + object() => %{ + references: list(object_reference()), + monitors: list(monitor()) + } + } + } + + @typedoc """ + Arbitrary term identifying an object. + """ + @type object :: term() + + @typedoc """ + Reference to an object with an optional scope. + """ + @type object_reference :: {process :: pid(), scope :: term()} + + @typedoc """ + Scheduled message to be sent when an object is released. + """ + @type monitor :: {Process.dest(), payload :: term()} + + @doc """ + Starts a new object tracker. + """ + @spec start_link(keyword()) :: GenServer.on_start() + def start_link(opts \\ []) do + GenServer.start_link(__MODULE__, opts) + end + + @doc """ + Adds a reference to the given object. + """ + @spec add_reference(pid(), object(), object_reference()) :: :ok + def add_reference(object_tracker, object, reference) do + GenServer.cast(object_tracker, {:add_reference, object, reference}) + end + + @doc """ + Removes the given reference from all objects it is attached to. + """ + @spec remove_reference(pid(), object_reference()) :: :ok + def remove_reference(object_tracker, reference) do + GenServer.cast(object_tracker, {:remove_reference, reference}) + end + + @doc """ + Schedules `payload` to be send to `destination` when the object + is released. + """ + @spec monitor(pid(), object(), Process.dest(), term()) :: :ok | {:error, :bad_object} + def monitor(object_tracker, object, destination, payload) do + GenServer.call(object_tracker, {:monitor, object, destination, payload}) + end + + @impl true + def init(_opts) do + {:ok, %{objects: %{}}} + end + + @impl true + def handle_cast({:add_reference, object, reference}, state) do + {parent, _scope} = reference + Process.monitor(parent) + + state = + if state.objects[object] do + update_in(state.objects[object].references, fn references -> + if reference in references, do: references, else: [reference | references] + end) + else + put_in(state.objects[object], %{references: [reference], monitors: []}) + end + + {:noreply, state} + end + + def handle_cast({:remove_reference, reference}, state) do + state = update_references(state, fn references -> List.delete(references, reference) end) + + {:noreply, garbage_collect(state)} + end + + @impl true + def handle_call({:monitor, object, destination, payload}, _from, state) do + monitor = {destination, payload} + + if state.objects[object] do + state = + update_in(state.objects[object].monitors, fn monitors -> + if monitor in monitors, do: monitors, else: [monitor | monitors] + end) + + {:reply, :ok, garbage_collect(state)} + else + {:reply, {:error, :bad_object}, state} + end + end + + @impl true + def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do + state = + update_references(state, fn references -> + Enum.reject(references, &match?({^pid, _}, &1)) + end) + + {:noreply, garbage_collect(state)} + end + + # Updates references for every object with the given function + defp update_references(state, fun) do + update_in(state.objects, fn objects -> + for {object, %{references: references} = info} <- objects, into: %{} do + {object, %{info | references: fun.(references)}} + end + end) + end + + defp garbage_collect(state) do + {to_release, objects} = Enum.split_with(state.objects, &match?({_, %{references: []}}, &1)) + + for {_, %{monitors: monitors}} <- to_release, {dest, payload} <- monitors do + send(dest, payload) + end + + %{state | objects: Map.new(objects)} + end +end diff --git a/lib/livebook/runtime/erl_dist.ex b/lib/livebook/runtime/erl_dist.ex index 300c8f0a0..1f96baa19 100644 --- a/lib/livebook/runtime/erl_dist.ex +++ b/lib/livebook/runtime/erl_dist.ex @@ -23,6 +23,7 @@ defmodule Livebook.Runtime.ErlDist do @required_modules [ Livebook.Evaluator, Livebook.Evaluator.IOProxy, + Livebook.Evaluator.ObjectTracker, Livebook.Evaluator.DefaultFormatter, Livebook.Intellisense, Livebook.Intellisense.IdentifierMatcher, diff --git a/lib/livebook/runtime/erl_dist/evaluator_supervisor.ex b/lib/livebook/runtime/erl_dist/evaluator_supervisor.ex index 7d1f4d175..c1233a58e 100644 --- a/lib/livebook/runtime/erl_dist/evaluator_supervisor.ex +++ b/lib/livebook/runtime/erl_dist/evaluator_supervisor.ex @@ -20,11 +20,11 @@ defmodule Livebook.Runtime.ErlDist.EvaluatorSupervisor do @doc """ Spawns a new evaluator. """ - @spec start_evaluator(pid()) :: {:ok, Evaluator.t()} | {:error, any()} - def start_evaluator(supervisor) do + @spec start_evaluator(pid(), pid()) :: {:ok, Evaluator.t()} | {:error, any()} + def start_evaluator(supervisor, object_tracker) do case DynamicSupervisor.start_child( supervisor, - {Evaluator, [formatter: Evaluator.DefaultFormatter]} + {Evaluator, [formatter: Evaluator.DefaultFormatter, object_tracker: object_tracker]} ) do {:ok, _pid, evaluator} -> {:ok, evaluator} {:error, reason} -> {:error, reason} diff --git a/lib/livebook/runtime/erl_dist/runtime_server.ex b/lib/livebook/runtime/erl_dist/runtime_server.ex index 4a6ac33d9..20f316171 100644 --- a/lib/livebook/runtime/erl_dist/runtime_server.ex +++ b/lib/livebook/runtime/erl_dist/runtime_server.ex @@ -115,13 +115,15 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do {:ok, evaluator_supervisor} = ErlDist.EvaluatorSupervisor.start_link() {:ok, completion_supervisor} = Task.Supervisor.start_link() + {:ok, object_tracker} = Livebook.Evaluator.ObjectTracker.start_link() {:ok, %{ owner: nil, evaluators: %{}, evaluator_supervisor: evaluator_supervisor, - completion_supervisor: completion_supervisor + completion_supervisor: completion_supervisor, + object_tracker: object_tracker }} end @@ -234,7 +236,12 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do if Map.has_key?(state.evaluators, container_ref) do state else - {:ok, evaluator} = ErlDist.EvaluatorSupervisor.start_evaluator(state.evaluator_supervisor) + {:ok, evaluator} = + ErlDist.EvaluatorSupervisor.start_evaluator( + state.evaluator_supervisor, + state.object_tracker + ) + Process.monitor(evaluator.pid) %{state | evaluators: Map.put(state.evaluators, container_ref, evaluator)} end diff --git a/lib/livebook/session.ex b/lib/livebook/session.ex index 098fb77c3..9183b5e8d 100644 --- a/lib/livebook/session.ex +++ b/lib/livebook/session.ex @@ -619,7 +619,7 @@ defmodule Livebook.Session do maybe_save_notebook_sync(state) broadcast_message(state.session_id, :session_closed) - {:stop, :shutdown, state} + {:stop, :normal, state} end @impl true diff --git a/lib/livebook_web/live/output.ex b/lib/livebook_web/live/output.ex index 2a07ef31b..c8a470a32 100644 --- a/lib/livebook_web/live/output.ex +++ b/lib/livebook_web/live/output.ex @@ -113,6 +113,10 @@ defmodule LivebookWeb.Output do ) end + defp render_output({:control, attrs}, %{id: id}) do + live_component(LivebookWeb.Output.ControlComponent, id: id, attrs: attrs) + end + defp render_output({:error, formatted, :runtime_restart_required}, %{runtime: runtime}) when runtime != nil do assigns = %{formatted: formatted, is_standalone: Livebook.Runtime.standalone?(runtime)} diff --git a/lib/livebook_web/live/output/control_component.ex b/lib/livebook_web/live/output/control_component.ex new file mode 100644 index 000000000..2728e93ff --- /dev/null +++ b/lib/livebook_web/live/output/control_component.ex @@ -0,0 +1,81 @@ +defmodule LivebookWeb.Output.ControlComponent do + use LivebookWeb, :live_component + + @impl true + def mount(socket) do + {:ok, assign(socket, keyboard_enabled: false)} + end + + @impl true + def render(%{attrs: %{type: :keyboard}} = assigns) do + ~H""" +
+ + + +
+ """ + end + + def render(%{attrs: %{type: :button}} = assigns) do + ~H""" +
+ +
+ """ + end + + def render(assigns) do + ~H""" +
+ Unknown control type <%= @attrs.type %> +
+ """ + end + + @impl true + def handle_event("toggle_keyboard", %{}, socket) do + socket = update(socket, :keyboard_enabled, ¬/1) + + if :status in socket.assigns.attrs.events do + report_event(socket, %{type: :status, enabled: socket.assigns.keyboard_enabled}) + end + + {:noreply, socket} + end + + def handle_event("button_click", %{}, socket) do + report_event(socket, %{type: :click}) + {:noreply, socket} + end + + def handle_event("keydown", %{"key" => key}, socket) do + report_event(socket, %{type: :keydown, key: key}) + {:noreply, socket} + end + + def handle_event("keyup", %{"key" => key}, socket) do + report_event(socket, %{type: :keyup, key: key}) + {:noreply, socket} + end + + defp report_event(socket, attrs) do + topic = socket.assigns.attrs.ref + event = Map.merge(%{origin: self()}, attrs) + send(socket.assigns.attrs.destination, {:event, topic, event}) + end +end diff --git a/lib/livebook_web/live/output/frame_dynamic.ex b/lib/livebook_web/live/output/frame_dynamic_live.ex similarity index 92% rename from lib/livebook_web/live/output/frame_dynamic.ex rename to lib/livebook_web/live/output/frame_dynamic_live.ex index dc40add2f..60da22733 100644 --- a/lib/livebook_web/live/output/frame_dynamic.ex +++ b/lib/livebook_web/live/output/frame_dynamic_live.ex @@ -3,7 +3,9 @@ defmodule LivebookWeb.Output.FrameDynamicLive do @impl true def mount(_params, %{"pid" => pid, "id" => id, "input_values" => input_values}, socket) do - send(pid, {:connect, self()}) + if connected?(socket) do + send(pid, {:connect, self()}) + end {:ok, assign(socket, id: id, output: nil, input_values: input_values)} end diff --git a/lib/livebook_web/live/output/input_component.ex b/lib/livebook_web/live/output/input_component.ex index d13fb33c2..d8730f89f 100644 --- a/lib/livebook_web/live/output/input_component.ex +++ b/lib/livebook_web/live/output/input_component.ex @@ -78,6 +78,8 @@ defmodule LivebookWeb.Output.InputComponent do name="value" value={@value} phx-debounce="300" + phx-blur="blur" + phx-target={@myself} spellcheck="false" autocomplete="off" min={@attrs.min} @@ -95,6 +97,8 @@ defmodule LivebookWeb.Output.InputComponent do class="input h-[200px] resize-none tiny-scrollbar" name="value" phx-debounce="300" + phx-blur="blur" + phx-target={@myself} spellcheck="false"><%= [?\n, @value] %> """ end @@ -108,13 +112,15 @@ defmodule LivebookWeb.Output.InputComponent do name="value" value={@value} phx-debounce="300" + phx-blur="blur" + phx-target={@myself} spellcheck="false" autocomplete="off" /> """ end - defp input(assigns) do + defp input(%{attrs: %{type: type}} = assigns) when type in [:number, :color, :url, :text] do ~H""" + phx-target={@myself} + spellcheck="false" + autocomplete="off" /> + """ + end + + defp input(assigns) do + ~H""" +
+ Unknown input type <%= @attrs.type %> +
""" end @@ -139,7 +153,9 @@ defmodule LivebookWeb.Output.InputComponent do {:noreply, handle_html_value(socket, html_value)} end - def handle_event("blur", %{}, socket) do + def handle_event("blur", %{"value" => html_value}, socket) do + socket = handle_html_value(socket, html_value) + if socket.assigns.error do {:noreply, assign(socket, value: socket.assigns.initial_value, error: nil)} else @@ -154,9 +170,15 @@ defmodule LivebookWeb.Output.InputComponent do end defp handle_html_value(socket, html_value) do + current_value = socket.assigns.value + case parse(html_value, socket.assigns.attrs) do + {:ok, ^current_value} -> + socket + {:ok, value} -> send(self(), {:set_input_value, socket.assigns.attrs.id, value}) + report_event(socket, value) assign(socket, value: value, error: nil) {:error, error, value} -> @@ -223,4 +245,10 @@ defmodule LivebookWeb.Output.InputComponent do defp parse(html_value, %{type: :color}) do {:ok, html_value} end + + defp report_event(socket, value) do + topic = socket.assigns.attrs.ref + event = %{value: value, origin: self(), type: :change} + send(socket.assigns.attrs.destination, {:event, topic, event}) + end end diff --git a/lib/livebook_web/live/output/table_dynamic_live.ex b/lib/livebook_web/live/output/table_dynamic_live.ex index f2d2e3cee..773209859 100644 --- a/lib/livebook_web/live/output/table_dynamic_live.ex +++ b/lib/livebook_web/live/output/table_dynamic_live.ex @@ -6,7 +6,9 @@ defmodule LivebookWeb.Output.TableDynamicLive do @impl true def mount(_params, %{"pid" => pid, "id" => id}, socket) do - send(pid, {:connect, self()}) + if connected?(socket) do + send(pid, {:connect, self()}) + end {:ok, assign(socket, diff --git a/lib/livebook_web/live/output/vega_lite_dynamic_live.ex b/lib/livebook_web/live/output/vega_lite_dynamic_live.ex index 959ab8851..5eb197923 100644 --- a/lib/livebook_web/live/output/vega_lite_dynamic_live.ex +++ b/lib/livebook_web/live/output/vega_lite_dynamic_live.ex @@ -3,7 +3,9 @@ defmodule LivebookWeb.Output.VegaLiteDynamicLive do @impl true def mount(_params, %{"pid" => pid, "id" => id}, socket) do - send(pid, {:connect, self()}) + if connected?(socket) do + send(pid, {:connect, self()}) + end {:ok, assign(socket, id: id)} end diff --git a/test/livebook/evaluator/io_proxy_test.exs b/test/livebook/evaluator/io_proxy_test.exs index c58e6b573..948de2eff 100644 --- a/test/livebook/evaluator/io_proxy_test.exs +++ b/test/livebook/evaluator/io_proxy_test.exs @@ -1,10 +1,15 @@ defmodule Livebook.Evaluator.IOProxyTest do use ExUnit.Case, async: true + alias Livebook.Evaluator alias Livebook.Evaluator.IOProxy setup do - {:ok, io} = IOProxy.start_link() + # {:ok, io} = IOProxy.start_link() + + {:ok, object_tracker} = start_supervised(Evaluator.ObjectTracker) + {:ok, _pid, evaluator} = start_supervised({Evaluator, [object_tracker: object_tracker]}) + io = Process.info(evaluator.pid)[:group_leader] IOProxy.configure(io, self(), :ref) %{io: io} end @@ -87,18 +92,6 @@ defmodule Livebook.Evaluator.IOProxyTest do assert_received {:evaluation_output, :ref, {:text, "[1, 2, 3]"}} end - test "flush_widgets/1 returns new widget pids", %{io: io} do - widget1_pid = IEx.Helpers.pid(0, 0, 0) - widget2_pid = IEx.Helpers.pid(0, 0, 1) - - livebook_put_output(io, {:vega_lite_dynamic, widget1_pid}) - livebook_put_output(io, {:vega_lite_dynamic, widget2_pid}) - livebook_put_output(io, {:vega_lite_dynamic, widget1_pid}) - - assert IOProxy.flush_widgets(io) == MapSet.new([widget1_pid, widget2_pid]) - assert IOProxy.flush_widgets(io) == MapSet.new() - end - describe "token requests" do test "returns different tokens for subsequent calls", %{io: io} do IOProxy.configure(io, self(), :ref1) diff --git a/test/livebook/evaluator/object_tracker_test.exs b/test/livebook/evaluator/object_tracker_test.exs new file mode 100644 index 000000000..5116a49e3 --- /dev/null +++ b/test/livebook/evaluator/object_tracker_test.exs @@ -0,0 +1,57 @@ +defmodule Livebook.Evaluator.ObjecTrackerTest do + use ExUnit.Case, async: true + + alias Livebook.Evaluator.ObjectTracker + + setup do + {:ok, object_tracker} = start_supervised(ObjectTracker) + %{object_tracker: object_tracker} + end + + test "monitor/4 returns an error when the given object doesn't exist", + %{object_tracker: object_tracker} do + assert {:error, :bad_object} = + ObjectTracker.monitor(object_tracker, :object1, self(), :object1_released) + end + + test "sends scheduled monitor messages when all object references are released", + %{object_tracker: object_tracker} do + ObjectTracker.add_reference(object_tracker, :object1, {self(), :ref1}) + ObjectTracker.add_reference(object_tracker, :object1, {self(), :ref2}) + + ObjectTracker.monitor(object_tracker, :object1, self(), :object1_released) + + ObjectTracker.remove_reference(object_tracker, {self(), :ref1}) + ObjectTracker.remove_reference(object_tracker, {self(), :ref2}) + + assert_receive :object1_released + end + + test "does not execute hooks when other references still point to the object", + %{object_tracker: object_tracker} do + ObjectTracker.add_reference(object_tracker, :object1, {self(), :ref1}) + ObjectTracker.add_reference(object_tracker, :object1, {self(), :ref2}) + + ObjectTracker.monitor(object_tracker, :object1, self(), :object1_released) + + ObjectTracker.remove_reference(object_tracker, {self(), :ref1}) + + refute_receive :object1_released + end + + test "removes a reference if its process terminates", %{object_tracker: object_tracker} do + reference_pid = + spawn(fn -> + receive do + :stop -> :ok + end + end) + + ObjectTracker.add_reference(object_tracker, :object1, {reference_pid, :ref1}) + + ObjectTracker.monitor(object_tracker, :object1, self(), :object1_released) + + send(reference_pid, :stop) + assert_receive :object1_released + end +end diff --git a/test/livebook/evaluator_test.exs b/test/livebook/evaluator_test.exs index 4fbe84aa2..7d0717bd2 100644 --- a/test/livebook/evaluator_test.exs +++ b/test/livebook/evaluator_test.exs @@ -4,8 +4,9 @@ defmodule Livebook.EvaluatorTest do alias Livebook.Evaluator setup do - {:ok, _pid, evaluator} = start_supervised(Evaluator) - %{evaluator: evaluator} + {:ok, object_tracker} = start_supervised(Evaluator.ObjectTracker) + {:ok, _pid, evaluator} = start_supervised({Evaluator, [object_tracker: object_tracker]}) + %{evaluator: evaluator, object_tracker: object_tracker} end describe "evaluate_code/6" do @@ -161,8 +162,9 @@ defmodule Livebook.EvaluatorTest do end test "kills widgets that that no evaluation points to", %{evaluator: evaluator} do - # Evaluate the code twice, which spawns two widget processes - # First of them should be eventually killed + # Evaluate the code twice, each time a new widget is spawned. + # The evaluation reference is the same, so the second one overrides + # the first one and the first widget should eventually be kiled. Evaluator.evaluate_code(evaluator, self(), spawn_widget_code(), :code_1) @@ -176,27 +178,26 @@ defmodule Livebook.EvaluatorTest do assert_receive {:evaluation_response, :code_1, {:ok, widget_pid2}, %{evaluation_time_ms: _time_ms}} - assert_receive {:DOWN, ^ref, :process, ^widget_pid1, :shutdown} + assert_receive {:DOWN, ^ref, :process, ^widget_pid1, _reason} assert Process.alive?(widget_pid2) end - test "does not kill a widget if another evaluation points to it", %{evaluator: evaluator} do - Evaluator.evaluate_code(evaluator, self(), spawn_widget_code(), :code_1) + test "kills widgets when the spawning process terminates", %{evaluator: evaluator} do + # The widget is spawned from a process that terminates, + # so the widget should terminate immediately as well + + Evaluator.evaluate_code( + evaluator, + self(), + spawn_widget_from_terminating_process_code(), + :code_1 + ) assert_receive {:evaluation_response, :code_1, {:ok, widget_pid1}, %{evaluation_time_ms: _time_ms}} - Evaluator.evaluate_code(evaluator, self(), spawn_widget_code(), :code_2) - - assert_receive {:evaluation_response, :code_2, {:ok, widget_pid2}, - %{evaluation_time_ms: _time_ms}} - - ref = Process.monitor(widget_pid1) - refute_receive {:DOWN, ^ref, :process, ^widget_pid1, :shutdown} - - assert Process.alive?(widget_pid1) - assert Process.alive?(widget_pid2) + refute Process.alive?(widget_pid1) end end @@ -225,7 +226,7 @@ defmodule Livebook.EvaluatorTest do ref = Process.monitor(widget_pid1) Evaluator.forget_evaluation(evaluator, :code_1) - assert_receive {:DOWN, ^ref, :process, ^widget_pid1, :shutdown} + assert_receive {:DOWN, ^ref, :process, ^widget_pid1, _reason} end end @@ -262,8 +263,10 @@ defmodule Livebook.EvaluatorTest do end describe "initialize_from/3" do - setup do - {:ok, _pid, parent_evaluator} = start_supervised(Evaluator, id: :parent_evaluator) + setup %{object_tracker: object_tracker} do + {:ok, _pid, parent_evaluator} = + start_supervised({Evaluator, [object_tracker: object_tracker]}, id: :parent_evaluator) + %{parent_evaluator: parent_evaluator} end @@ -299,16 +302,20 @@ defmodule Livebook.EvaluatorTest do :ok end - # Returns a code that spawns and renders a widget process - # and returns its pid from the evaluation + # Returns a code that spawns a widget process, registers + # a pointer for it and adds monitoring, then returns widget + # pid from the evaluation defp spawn_widget_code() do """ widget_pid = spawn(fn -> - Process.sleep(:infinity) + receive do + :stop -> :ok + end end) ref = make_ref() - send(Process.group_leader(), {:io_request, self(), ref, {:livebook_put_output, {:vega_lite_dynamic, widget_pid}}}) + send(Process.group_leader(), {:io_request, self(), ref, {:livebook_reference_object, widget_pid, self()}}) + send(Process.group_leader(), {:io_request, self(), ref, {:livebook_monitor_object, widget_pid, widget_pid, :stop}}) receive do {:io_reply, ^ref, :ok} -> :ok @@ -317,4 +324,33 @@ defmodule Livebook.EvaluatorTest do widget_pid """ end + + defp spawn_widget_from_terminating_process_code() do + """ + parent = self() + + # Arbitrary process that spawns the widget and terminates afterwards + spawn(fn -> + widget_pid = spawn(fn -> + receive do + :stop -> :ok + end + end) + + ref = make_ref() + send(Process.group_leader(), {:io_request, self(), ref, {:livebook_reference_object, widget_pid, self()}}) + send(Process.group_leader(), {:io_request, self(), ref, {:livebook_monitor_object, widget_pid, widget_pid, :stop}}) + + receive do + {:io_reply, ^ref, :ok} -> :ok + end + + send(parent, {:widget_pid, widget_pid}) + end) + + receive do + {:widget_pid, widget_pid} -> widget_pid + end + """ + end end diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index d0413854f..89e865e13 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -167,14 +167,18 @@ defmodule LivebookWeb.SessionLiveTest do assert %{notebook: %{sections: [%{cells: []}]}} = Session.get_data(session.pid) end - test "editing input field in cell output", %{conn: conn, session: session} do + test "editing input field in cell output", %{conn: conn, session: session, test: test} do section_id = insert_section(session.pid) + Process.register(self(), test) + insert_cell_with_input(session.pid, section_id, %{ + ref: :reference, id: "input1", type: :number, label: "Name", - default: "hey" + default: "hey", + destination: test }) {:ok, view, _} = live(conn, "/sessions/#{session.id}") @@ -186,14 +190,18 @@ defmodule LivebookWeb.SessionLiveTest do assert %{input_values: %{"input1" => 10}} = Session.get_data(session.pid) end - test "newlines in text input are normalized", %{conn: conn, session: session} do + test "newlines in text input are normalized", %{conn: conn, session: session, test: test} do section_id = insert_section(session.pid) + Process.register(self(), test) + insert_cell_with_input(session.pid, section_id, %{ + ref: :reference, id: "input1", type: :textarea, label: "Name", - default: "hey" + default: "hey", + destination: test }) {:ok, view, _} = live(conn, "/sessions/#{session.id}") @@ -705,7 +713,8 @@ defmodule LivebookWeb.SessionLiveTest do quote do send( Process.group_leader(), - {:io_request, self(), make_ref(), {:livebook_put_output, {:input, unquote(input)}}} + {:io_request, self(), make_ref(), + {:livebook_put_output, {:input, unquote(Macro.escape(input))}}} ) end |> Macro.to_string()