From 46d1df48f8da6efe961d86196a531392c6490815 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 18 Jan 2023 15:04:20 +0100 Subject: [PATCH 01/33] Add `handle_child_pad_removed/4` callback in bins and pipelines --- lib/membrane/bin.ex | 25 +++++- lib/membrane/core/bin.ex | 5 ++ lib/membrane/core/bin/pad_controller.ex | 16 ++++ .../core/parent/child_life_controller.ex | 18 ++++ .../child_life_controller/link_utils.ex | 90 ++++++++++++++----- lib/membrane/core/pipeline.ex | 6 ++ lib/membrane/exceptions.ex | 18 ++++ lib/membrane/pipeline.ex | 24 ++++- 8 files changed, 173 insertions(+), 29 deletions(-) diff --git a/lib/membrane/bin.ex b/lib/membrane/bin.ex index 9d9ac69c8..3eb51ca80 100644 --- a/lib/membrane/bin.ex +++ b/lib/membrane/bin.ex @@ -90,6 +90,20 @@ defmodule Membrane.Bin do ) :: callback_return + @doc """ + Callback invoked when a child removes its pad. + + Removing child's pad due to return `t:Membrane.Bin.Action.remove_children()` + or `t:Membrane.Bin.Action.remove_link()` action from parent's callback does + not trigger this callback. + """ + @callback handle_child_pad_removed( + child :: Child.name(), + pad :: Pad.ref(), + context :: CallbackContext.t(), + state :: state + ) :: callback_return + @doc """ Callback invoked when a notification comes in from an element. """ @@ -168,8 +182,7 @@ defmodule Membrane.Bin do @callback handle_terminate_request( context :: CallbackContext.t(), state - ) :: - callback_return() + ) :: callback_return @optional_callbacks handle_init: 2, handle_pad_added: 3, @@ -326,6 +339,11 @@ defmodule Membrane.Bin do @impl true def handle_terminate_request(_ctx, state), do: {[terminate: :normal], state} + @impl true + def handle_child_pad_removed(child, pad, _ctx, _state) do + raise Membrane.ChildPadRemovedError, child: child, pad: pad, module: __MODULE__ + end + defoverridable handle_init: 2, handle_pad_added: 3, handle_pad_removed: 3, @@ -337,7 +355,8 @@ defmodule Membrane.Bin do handle_element_end_of_stream: 4, handle_child_notification: 4, handle_parent_notification: 3, - handle_terminate_request: 2 + handle_terminate_request: 2, + handle_child_pad_removed: 4 end end diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index e552daebf..e5950b72a 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -183,6 +183,11 @@ defmodule Membrane.Core.Bin do {:noreply, state} end + defp do_handle_info(Message.new(:child_pad_removed, [child, pad]), state) do + state = Parent.ChildLifeController.handle_child_pad_removed(child, pad, state) + {:noreply, state} + end + defp do_handle_info(Message.new(:child_notification, [from, notification]), state) do state = Parent.LifecycleController.handle_child_notification(from, notification, state) {:noreply, state} diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index 9571aafcd..3198b7a7d 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -80,6 +80,22 @@ defmodule Membrane.Core.Bin.PadController do state end + @spec remove_dynamic_pad(Pad.ref(), State.t()) :: State.t() + def remove_dynamic_pad(pad_ref, state) do + case pad_ref do + Pad.ref(_name, _id) -> + Message.send(state.parent_pid, :child_pad_removed, [state.name, pad_ref]) + PadModel.delete_data!(state, pad_ref) + + name when is_atom(name) and state.terminating? -> + state + + name when is_atom(name) -> + raise Membrane.PadError, + "Tried to unlink bin static pad #{inspect(pad_ref)}. Static pads cannot be unlinked unless bin is terminating" + end + end + @spec handle_linking_timeout(Pad.ref(), State.t()) :: :ok | no_return() def handle_linking_timeout(pad_ref, state) do case PadModel.get_data(state, pad_ref) do diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 29941e00e..93957de2a 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -532,6 +532,24 @@ defmodule Membrane.Core.Parent.ChildLifeController do LinkUtils.remove_link(child_name, pad_ref, state) end + @spec handle_child_pad_removed(Child.name(), Pad.ref(), PArent.state()) :: Parent.state() + def handle_child_pad_removed(child, pad, state) do + Membrane.Logger.debug_verbose("Child #{inspect(child)} removed pad #{inspect(pad)}") + + Parent.ChildrenModel.assert_child_exists!(state, child) + + state = + CallbackHandler.exec_and_handle_callback( + :handle_child_pad_removed, + Component.action_handler(state), + %{context: &Component.context_from_state/1}, + [child, pad], + state + ) + + LinkUtils.handle_child_pad_removed(child, pad, state) + end + @doc """ Handles death of a child: - removes it from state diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 04aa58418..686c7052d 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -33,22 +33,41 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end) end + def handle_child_pad_removed(child, pad, state) do + {:ok, link} = get_link(state.links, child, pad) + + opposite_endpoint(link, child) + |> case do + %Endpoint{child: {Membrane.Bin, :itself}} = bin_endpoint -> + PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) + + %Endpoint{} = endpoint -> + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + state + end + |> delete_link(link.id) + end + @spec remove_link(Membrane.Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def remove_link(child_name, pad_ref, state) do - Enum.find(state.links, fn {_id, link} -> - [link.from, link.to] - |> Enum.any?(&(&1.child == child_name and &1.pad_ref == pad_ref)) - end) - |> case do - {_id, %Link{} = link} -> + with {:ok, link} <- get_link(state.links, child_name, pad_ref) do + if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do + child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) + Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) + + bin_endpoint = opposite_endpoint(link, child_endpoint.child) + state = PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) + + delete_link(state, link.id) + else for endpoint <- [link.from, link.to] do Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) end - links = Map.delete(state.links, link.id) - Map.put(state, :links, links) - - nil -> + delete_link(state, link.id) + end + else + {:error, :not_found} -> with %{^child_name => _child_entry} <- state.children do raise ParentError, """ Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but this child does not have this pad linked @@ -63,20 +82,23 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do @spec unlink_element(Membrane.Child.name(), Parent.state()) :: Parent.state() def unlink_element(child_name, state) do - Map.update!( - state, - :links, - &Map.reject(&1, fn {_id, %Link{} = link} -> - case endpoint_to_unlink(child_name, link) do - %Endpoint{pid: pid, pad_ref: pad_ref} -> - Message.send(pid, :handle_unlink, pad_ref) - true - - nil -> - false - end - end) - ) + {dropped_links, links} = + state.links + |> Map.values() + |> Enum.split_with(&(child_name in [&1.from.child, &1.to.child])) + + state = %{state | links: Map.new(links, &{&1.id, &1})} + + Enum.reduce(dropped_links, state, fn link, state -> + case endpoint_to_unlink(child_name, link) do + %Endpoint{child: {Membrane.Bin, :itself}, pad_ref: pad_ref} -> + PadController.remove_dynamic_pad(pad_ref, state) + + %Endpoint{pid: pid, pad_ref: pad_ref} -> + Message.send(pid, :handle_unlink, pad_ref) + state + end + end) end defp endpoint_to_unlink(child_name, %Link{from: %Endpoint{child: child_name}, to: to}), do: to @@ -145,6 +167,26 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do links end + defp get_link(links, child, pad) do + Enum.find(links, fn {_id, link} -> + [link.from, link.to] + |> Enum.any?(&(&1.child == child and &1.pad_ref == pad)) + end) + |> case do + {_id, %Link{} = link} -> {:ok, link} + nil -> {:error, :not_found} + end + end + + defp opposite_endpoint(%Link{from: %Endpoint{child: child}, to: to}, child), do: to + + defp opposite_endpoint(%Link{to: %Endpoint{child: child}, from: from}, child), do: from + + defp delete_link(state, link_id) do + links = Map.delete(state.links, link_id) + Map.put(state, :links, links) + end + defp validate_links(links, state) do links |> Enum.concat(Map.values(state.links)) diff --git a/lib/membrane/core/pipeline.ex b/lib/membrane/core/pipeline.ex index 18f978416..28b25da77 100644 --- a/lib/membrane/core/pipeline.ex +++ b/lib/membrane/core/pipeline.ex @@ -73,6 +73,12 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end + @impl GenServer + def handle_info(Message.new(:child_pad_removed, [child, pad]), state) do + state = ChildLifeController.handle_child_pad_removed(child, pad, state) + {:noreply, state} + end + @impl GenServer def handle_info(Message.new(:child_notification, [from, notification]), state) do state = LifecycleController.handle_child_notification(from, notification, state) diff --git a/lib/membrane/exceptions.ex b/lib/membrane/exceptions.ex index df28edb6f..9dd42e6a1 100644 --- a/lib/membrane/exceptions.ex +++ b/lib/membrane/exceptions.ex @@ -156,3 +156,21 @@ defmodule Membrane.PadDirectionError do } end end + +defmodule Membrane.ChildPadRemovedError do + defexception [:message] + + @impl true + def exception(opts) do + child = Keyword.fetch!(opts, :child) + pad = Keyword.fetch!(opts, :pad) + module = Keyword.fetch!(opts, :module) + + msg = """ + Child #{inspect(child)} removed pad #{inspect(pad)}, but `handle_child_pad_removed/4` + is not implememnted in parent's module (#{inspect(module)}) + """ + + %__MODULE__{message: msg} + end +end diff --git a/lib/membrane/pipeline.ex b/lib/membrane/pipeline.ex index 4272742bd..c6cf5bf7d 100644 --- a/lib/membrane/pipeline.ex +++ b/lib/membrane/pipeline.ex @@ -29,7 +29,7 @@ defmodule Membrane.Pipeline do use Membrane.Pipeline def start_link(options) do - Membrane.Pipeline.start_link(options, name: MyPipeline) + Membrane.Pipeline.start_link(__MODULE__, options, name: MyPipeline) end # ... @@ -135,6 +135,20 @@ defmodule Membrane.Pipeline do ) :: callback_return + @doc """ + Callback invoked when a child removes its pad. + + Removing child's pad due to return `t:Membrane.Bin.Action.remove_children()` + or `t:Membrane.Bin.Action.remove_link()` action from parent's callback does + not trigger this callback. + """ + @callback handle_child_pad_removed( + child :: Child.name(), + pad :: Pad.ref(), + context :: CallbackContext.t(), + state :: state + ) :: callback_return + @doc """ Callback invoked when a notification comes in from an element. """ @@ -471,6 +485,11 @@ defmodule Membrane.Pipeline do @impl true def handle_terminate_request(_ctx, state), do: {[terminate: :normal], state} + @impl true + def handle_child_pad_removed(child, pad, _ctx, _state) do + raise Membrane.ChildPadRemovedError, child: child, pad: pad, module: __MODULE__ + end + defoverridable child_spec: 1, handle_init: 2, handle_setup: 2, @@ -482,7 +501,8 @@ defmodule Membrane.Pipeline do handle_child_notification: 4, handle_crash_group_down: 3, handle_call: 3, - handle_terminate_request: 2 + handle_terminate_request: 2, + handle_child_pad_removed: 4 end end end From 81cc84db29374c356ab2d26d76eaa68d05599833 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 19 Jan 2023 16:52:44 +0100 Subject: [PATCH 02/33] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9377e9d19..6d6058f74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ * Rename callbacks `handle_process/4` and `handle_write/4` to `handle_buffer/4` in [#506](https://github.com/membraneframework/membrane_core/pull/506) * The flow control of the pad is now set with a single `:flow_control` option instead of `:mode` and `:demand_mode` options. * Remove _t suffix from types [#509](https://github.com/membraneframework/membrane_core/pull/509) + * Add `handle_child_pad_removed/4` callback in Bins and Pipelines. [#513](https://github.com/membraneframework/membrane_core/pull/513) ## 0.11.0 * Separate element_name and pad arguments in handle_element_{start, end}_of_stream signature [#219](https://github.com/membraneframework/membrane_core/issues/219) From fa27e4bde5aa62d7480f7e3a8583a91142bfc463 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 19 Jan 2023 16:55:53 +0100 Subject: [PATCH 03/33] Fix credo issue --- .../core/parent/child_life_controller/link_utils.ex | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 686c7052d..0641200bb 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -3,7 +3,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do use Bunch - alias Membrane.ParentError + alias Membrane.Child alias Membrane.Core.{Bin, Message, Parent, Telemetry} alias Membrane.Core.Bin.PadController @@ -18,6 +18,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do alias Membrane.Core.Parent.Link.Endpoint alias Membrane.LinkError alias Membrane.Pad + alias Membrane.ParentError require Membrane.Core.Message require Membrane.Core.Telemetry @@ -33,6 +34,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end) end + @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do {:ok, link} = get_link(state.links, child, pad) @@ -48,7 +50,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do |> delete_link(link.id) end - @spec remove_link(Membrane.Child.name(), Pad.ref(), Parent.state()) :: Parent.state() + @spec remove_link(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def remove_link(child_name, pad_ref, state) do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do @@ -80,7 +82,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end end - @spec unlink_element(Membrane.Child.name(), Parent.state()) :: Parent.state() + @spec unlink_element(Child.name(), Parent.state()) :: Parent.state() def unlink_element(child_name, state) do {dropped_links, links} = state.links From df5bd3e898d20d117ccadc712eabe5a34a2007ee Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 19 Jan 2023 17:08:16 +0100 Subject: [PATCH 04/33] Fix dialyzer issues --- lib/membrane/bin.ex | 11 ++---- .../core/parent/child_life_controller.ex | 38 +++++++++---------- lib/membrane/pipeline.ex | 11 ++---- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/lib/membrane/bin.ex b/lib/membrane/bin.ex index 3eb51ca80..c7546c256 100644 --- a/lib/membrane/bin.ex +++ b/lib/membrane/bin.ex @@ -196,7 +196,8 @@ defmodule Membrane.Bin do handle_child_notification: 4, handle_parent_notification: 3, handle_tick: 3, - handle_terminate_request: 2 + handle_terminate_request: 2, + handle_child_pad_removed: 4 @doc PadsSpecs.def_pad_docs(:input, :bin) defmacro def_input_pad(name, spec) do @@ -339,11 +340,6 @@ defmodule Membrane.Bin do @impl true def handle_terminate_request(_ctx, state), do: {[terminate: :normal], state} - @impl true - def handle_child_pad_removed(child, pad, _ctx, _state) do - raise Membrane.ChildPadRemovedError, child: child, pad: pad, module: __MODULE__ - end - defoverridable handle_init: 2, handle_pad_added: 3, handle_pad_removed: 3, @@ -355,8 +351,7 @@ defmodule Membrane.Bin do handle_element_end_of_stream: 4, handle_child_notification: 4, handle_parent_notification: 3, - handle_terminate_request: 2, - handle_child_pad_removed: 4 + handle_terminate_request: 2 end end diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 93957de2a..608d19a94 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -3,7 +3,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do use Bunch alias __MODULE__.{CrashGroupUtils, LinkUtils, StartupUtils} - alias Membrane.ChildrenSpec + alias Membrane.{Child, ChildrenSpec} alias Membrane.Core.{Bin, CallbackHandler, Component, Parent, Pipeline} alias Membrane.Core.Parent.{ @@ -31,7 +31,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do | :linked_internally | :linking_externally | :ready, - children_names: [Membrane.Child.name()], + children_names: [Child.name()], links_ids: [Link.id()], awaiting_responses: MapSet.t({Link.id(), Membrane.Pad.direction()}), dependent_specs: MapSet.t(spec_ref) @@ -40,16 +40,16 @@ defmodule Membrane.Core.Parent.ChildLifeController do @type pending_specs :: %{spec_ref() => pending_spec()} @opaque parsed_children_spec_options :: %{ - group: Membrane.Child.group(), + group: Child.group(), crash_group_mode: Membrane.CrashGroup.mode(), - stream_sync: :sinks | [[Membrane.Child.name()]], - clock_provider: Membrane.Child.name() | nil, + stream_sync: :sinks | [[Child.name()]], + clock_provider: Child.name() | nil, node: node() | nil, log_metadata: Keyword.t() } @type children_spec_canonical_form :: [ - {[Membrane.ChildrenSpec.builder()], parsed_children_spec_options()} + {[ChildrenSpec.builder()], parsed_children_spec_options()} ] @spec_dependency_requiring_statuses [:initializing, :linking_internally] @@ -73,7 +73,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do } @doc """ - Handles `Membrane.ChildrenSpec` returned with `spec` action. + Handles `ChildrenSpec` returned with `spec` action. Handling a spec consists of the following steps: - Parse the spec @@ -196,7 +196,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end end - @spec make_canonical(Membrane.ChildrenSpec.t(), parsed_children_spec_options()) :: + @spec make_canonical(ChildrenSpec.t(), parsed_children_spec_options()) :: children_spec_canonical_form() defp make_canonical(spec, defaults \\ @default_children_spec_options) @@ -261,7 +261,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do defp get_child_ref(child_name_or_ref, group) do case child_name_or_ref do # child name created with child(...) - {:child_name, child_name} -> Membrane.Child.ref(child_name, group: group) + {:child_name, child_name} -> Child.ref(child_name, group: group) # child name created with get_child(...), bin_input() and bin_output() {:child_ref, ref} -> ref end @@ -469,7 +469,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end end - @spec handle_child_initialized(Membrane.Child.name(), Parent.state()) :: Parent.state() + @spec handle_child_initialized(Child.name(), Parent.state()) :: Parent.state() def handle_child_initialized(child, state) do %{spec_ref: spec_ref} = Parent.ChildrenModel.get_child_data!(state, child) state = put_in(state, [:children, child, :initialized?], true) @@ -477,7 +477,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end @spec handle_notify_child( - {Membrane.Child.name(), Membrane.ParentNotification.t()}, + {Child.name(), Membrane.ParentNotification.t()}, Parent.state() ) :: :ok def handle_notify_child({child_name, message}, state) do @@ -487,10 +487,10 @@ defmodule Membrane.Core.Parent.ChildLifeController do end @spec handle_remove_children( - Membrane.Child.ref() - | [Membrane.Child.ref()] - | Membrane.Child.group() - | [Membrane.Child.group()], + Child.ref() + | [Child.ref()] + | Child.group() + | [Child.group()], Parent.state() ) :: Parent.state() def handle_remove_children(children_or_children_groups, state) do @@ -526,13 +526,13 @@ defmodule Membrane.Core.Parent.ChildLifeController do Parent.ChildrenModel.update_children!(state, refs, &%{&1 | terminating?: true}) end - @spec handle_remove_link(Membrane.Child.name(), Pad.ref(), Parent.state()) :: + @spec handle_remove_link(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_remove_link(child_name, pad_ref, state) do LinkUtils.remove_link(child_name, pad_ref, state) end - @spec handle_child_pad_removed(Child.name(), Pad.ref(), PArent.state()) :: Parent.state() + @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do Membrane.Logger.debug_verbose("Child #{inspect(child)} removed pad #{inspect(pad)}") @@ -557,7 +557,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do - handles crash group (if applicable) """ @spec handle_child_death( - child_name :: Membrane.Child.name(), + child_name :: Child.name(), reason :: any(), state :: Parent.state() ) :: {:stop | :continue, Parent.state()} @@ -672,7 +672,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end # called when process was a member of a crash group - @spec crash_all_group_members(CrashGroup.t(), Membrane.Child.name(), Parent.state()) :: + @spec crash_all_group_members(CrashGroup.t(), Child.name(), Parent.state()) :: Parent.state() defp crash_all_group_members( %CrashGroup{triggered?: false} = crash_group, diff --git a/lib/membrane/pipeline.ex b/lib/membrane/pipeline.ex index c6cf5bf7d..a897576e3 100644 --- a/lib/membrane/pipeline.ex +++ b/lib/membrane/pipeline.ex @@ -245,7 +245,8 @@ defmodule Membrane.Pipeline do handle_tick: 3, handle_crash_group_down: 3, handle_call: 3, - handle_terminate_request: 2 + handle_terminate_request: 2, + handle_child_pad_removed: 4 @doc """ Starts the Pipeline based on given module and links it to the current @@ -485,11 +486,6 @@ defmodule Membrane.Pipeline do @impl true def handle_terminate_request(_ctx, state), do: {[terminate: :normal], state} - @impl true - def handle_child_pad_removed(child, pad, _ctx, _state) do - raise Membrane.ChildPadRemovedError, child: child, pad: pad, module: __MODULE__ - end - defoverridable child_spec: 1, handle_init: 2, handle_setup: 2, @@ -501,8 +497,7 @@ defmodule Membrane.Pipeline do handle_child_notification: 4, handle_crash_group_down: 3, handle_call: 3, - handle_terminate_request: 2, - handle_child_pad_removed: 4 + handle_terminate_request: 2 end end end From 54f8a4e43a5aae15689250454481d9a13261bbcb Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 24 Jan 2023 18:57:48 +0100 Subject: [PATCH 05/33] Write tests for handle_child_pad_removed --- .../core/parent/child_life_controller.ex | 1 + .../child_life_controller/link_utils.ex | 38 +++- test/membrane/element_test.exs | 1 - .../integration/child_pad_removed_test.exs | 203 ++++++++++++++++++ 4 files changed, 231 insertions(+), 12 deletions(-) create mode 100644 test/membrane/integration/child_pad_removed_test.exs diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 608d19a94..f335f7ea3 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -534,6 +534,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do + # TODO: when spec is not ready yet, delete specific link from it and trigger proceeding Membrane.Logger.debug_verbose("Child #{inspect(child)} removed pad #{inspect(pad)}") Parent.ChildrenModel.assert_child_exists!(state, child) diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 0641200bb..e09b402a4 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -44,10 +44,10 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + maybe_send_handle_unlink(endpoint, state) state end - |> delete_link(link.id) + |> delete_link(link) end @spec remove_link(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() @@ -55,18 +55,18 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) + maybe_send_handle_unlink(child_endpoint, state) bin_endpoint = opposite_endpoint(link, child_endpoint.child) state = PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) - delete_link(state, link.id) + delete_link(state, link) else for endpoint <- [link.from, link.to] do - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + maybe_send_handle_unlink(endpoint, state) end - delete_link(state, link.id) + delete_link(state, link) end else {:error, :not_found} -> @@ -96,8 +96,8 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do %Endpoint{child: {Membrane.Bin, :itself}, pad_ref: pad_ref} -> PadController.remove_dynamic_pad(pad_ref, state) - %Endpoint{pid: pid, pad_ref: pad_ref} -> - Message.send(pid, :handle_unlink, pad_ref) + %Endpoint{} = endpoint -> + maybe_send_handle_unlink(endpoint, state) state end end) @@ -184,9 +184,19 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do defp opposite_endpoint(%Link{to: %Endpoint{child: child}, from: from}, child), do: from - defp delete_link(state, link_id) do - links = Map.delete(state.links, link_id) - Map.put(state, :links, links) + defp delete_link(state, link) do + links = Map.delete(state.links, link.id) + state = Map.put(state, :links, links) + + spec_ref = link.spec_ref + + with %{^spec_ref => spec_data} <- state.pending_specs do + new_links_ids = Enum.reject(spec_data.links_ids, &(&1 == link.id)) + state = put_in(state, [:pending_specs, spec_ref, :links_ids], new_links_ids) + ChildLifeController.proceed_spec_startup(spec_ref, state) + else + _pending_specs -> state + end end defp validate_links(links, state) do @@ -317,4 +327,10 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end end end + + defp maybe_send_handle_unlink(%Endpoint{child: child} = endpoint, state) do + with %{^child => %{terminating?: false}} <- state.children do + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + end + end end diff --git a/test/membrane/element_test.exs b/test/membrane/element_test.exs index baf206417..bcd7075ce 100644 --- a/test/membrane/element_test.exs +++ b/test/membrane/element_test.exs @@ -99,7 +99,6 @@ defmodule Membrane.ElementTest do end describe "End of stream" do - @tag :target test "causes handle_end_of_stream/3 to be called", %{pipeline: pipeline} do assert_pipeline_play(pipeline) diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs new file mode 100644 index 000000000..b9afc9c6f --- /dev/null +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -0,0 +1,203 @@ +defmodule Membrane.Integration.ChildPadRemovedTest do + use ExUnit.Case, async: false + + alias Membrane.Testing + + require Membrane.Pad, as: Pad + + defmodule DynamicSource do + use Membrane.Source + def_output_pad :output, flow_control: :push, availability: :on_request, accepted_format: _any + end + + defmodule DynamicSink do + use Membrane.Sink + def_input_pad :input, flow_control: :push, availability: :on_request, accepted_format: _any + def_options test_process: [spec: pid()] + + @impl true + def handle_pad_added(_pad, ctx, state) do + send(state.test_process, {:pad_added, ctx.name}) + {[], state} + end + + @impl true + def handle_pad_removed(_pad, ctx, state) do + send(state.test_process, {:pad_removed, ctx.name}) + {[], state} + end + end + + defmodule StaticSink do + use Membrane.Sink + def_input_pad :input, flow_control: :push, accepted_format: _any + def_options test_process: [spec: pid()] + end + + defmodule DynamicBin do + use Membrane.Bin + + require Membrane.Pad, as: Pad + + def_output_pad :output, availability: :on_request, accepted_format: _any + def_options test_process: [spec: pid()] + + @impl true + def handle_parent_notification({:execute_actions, actions}, _ctx, state) do + {actions, state} + end + + @impl true + def handle_pad_added(pad, _ctx, state) do + spec = + child(:source, DynamicSource) + |> via_out(Pad.ref(:output, 1)) + |> bin_output(pad) + + {[spec: spec], state} + end + end + + defmodule Pipeline do + use Membrane.Pipeline + + @impl true + def handle_init(_ctx, opts) do + %{bin: bin, sink: sink, actions_on_child_removed_pad: actions, test_process: test_process} = + Map.new(opts) + + spec = child(:bin, bin) |> child(:sink, sink) + {[spec: spec], %{actions: actions, test_process: test_process}} + end + + @impl true + def handle_info({:execute_actions, actions}, _ctx, state) do + {actions, state} + end + + @impl true + def handle_child_pad_removed(:bin, pad, _ctx, %{actions: actions} = state) do + send(state.test_process, {:child_pad_removed, :bin, pad}) + {actions, state} + end + end + + defp start_pipeline!(bin, sink, actions_on_child_removed_pad \\ []) do + do_start_pipeline!(:start, bin, sink, actions_on_child_removed_pad) + end + + defp start_link_pipeline!(bin, sink, actions_on_child_removed_pad \\ []) do + do_start_pipeline!(:start_link, bin, sink, actions_on_child_removed_pad) + end + + defp do_start_pipeline!(function, bin, sink, actions) do + args = [ + Pipeline, + [ + bin: struct(bin, test_process: self()), + sink: struct(sink, test_process: self()), + actions_on_child_removed_pad: actions, + test_process: self() + ] + ] + + {:ok, _supervisor, pipeline} = apply(Membrane.Pipeline, function, args) + + pipeline + end + + defp execute_actions_in_bin(pipeline, actions) do + msg_to_bin = {:execute_actions, actions} + msg_to_pipeline = {:execute_actions, notify_child: {:bin, msg_to_bin}} + send(pipeline, msg_to_pipeline) + end + + defp assert_child_exists(pipeline, child_ref_path) do + assert {:ok, pid} = Testing.Pipeline.get_child_pid(pipeline, child_ref_path) + assert is_pid(pid) + end + + describe "when child-bin removes a pad" do + test "sibling is unlinked" do + for bin_actions <- [ + [remove_children: :source], + [remove_link: {:source, Pad.ref(:output, 1)}] + ] do + pipeline = start_link_pipeline!(DynamicBin, DynamicSink) + + # This sleep is executed to ensure, that spec has already been finished. + # After fixing bug occuring, when parent returns :remove_link before + # spec status is :ready, this sleep will become unneceassary. + Process.sleep(100) + execute_actions_in_bin(pipeline, bin_actions) + + receive do + {:pad_added, :sink} -> + assert_receive {:pad_removed, :sink} + after + 500 -> + refute_received {:pad_removed, :sink} + end + + assert_receive {:child_pad_removed, :bin, Pad.ref(:output, _id)} + + assert_child_exists(pipeline, :bin) + assert_child_exists(pipeline, :sink) + + Pipeline.terminate(pipeline, blocking?: true) + end + end + + test "sibling linked via static pad raises" do + for actions <- [ + [remove_children: :source], + [remove_link: {:source, Pad.ref(:output, 1)}] + ] do + pipeline = start_pipeline!(DynamicBin, StaticSink) + + # This sleep is executed to ensure, that spec has already been finished. + # After fixing bug occuring, when parent returns :remove_link before + # spec status is :ready, this sleep will become unneceassary. + Process.sleep(100) + + sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) + monitor_ref = Process.monitor(sink_pid) + + execute_actions_in_bin(pipeline, actions) + + assert_receive {:child_pad_removed, :bin, Pad.ref(:output, _id)} + + assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, + {%Membrane.PadError{message: message}, _stacktrace}} + + assert message =~ ~r/Tried.*to.*unlink.*a.*static.*pad.*input.*/ + end + end + + @tag :target + test "and sibling linked via static pad is removed, pipeline is not raising" do + for bin_actions <- [ + [remove_children: :source], + [remove_link: {:source, Pad.ref(:output, 1)}] + ] do + pipeline = start_link_pipeline!(DynamicBin, StaticSink, remove_children: :sink) + + # This sleep is executed to ensure, that spec has already been finished. + # After fixing bug occuring, when parent returns :remove_link before + # spec status is :ready, this sleep will become unneceassary. + Process.sleep(100) + + sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) + monitor_ref = Process.monitor(sink_pid) + + execute_actions_in_bin(pipeline, bin_actions) + + assert_receive {:child_pad_removed, :bin, Pad.ref(:output, _id)} + assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, _reason} + + assert_child_exists(pipeline, :bin) + Pipeline.terminate(pipeline, blocking?: true) + end + end + end +end From 835cac688381b9dc478545ac2f4ec47a07b62eee Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 25 Jan 2023 15:40:10 +0100 Subject: [PATCH 06/33] Send :handle_unlink, even if child is terminating --- .../parent/child_life_controller/link_utils.ex | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index e09b402a4..d3a0827dd 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -44,7 +44,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> - maybe_send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) state end |> delete_link(link) @@ -55,7 +55,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - maybe_send_handle_unlink(child_endpoint, state) + Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) bin_endpoint = opposite_endpoint(link, child_endpoint.child) state = PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) @@ -63,7 +63,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do delete_link(state, link) else for endpoint <- [link.from, link.to] do - maybe_send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) end delete_link(state, link) @@ -97,7 +97,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad(pad_ref, state) %Endpoint{} = endpoint -> - maybe_send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) state end end) @@ -327,10 +327,4 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end end end - - defp maybe_send_handle_unlink(%Endpoint{child: child} = endpoint, state) do - with %{^child => %{terminating?: false}} <- state.children do - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) - end - end end From cf7a11cf605e6b09faed63fdb6fdf28d75b4619b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 25 Jan 2023 16:15:32 +0100 Subject: [PATCH 07/33] Refactor handle_child_pad_removed callback related code --- lib/membrane/bin.ex | 5 ++--- lib/membrane/core/bin/pad_controller.ex | 4 ++-- lib/membrane/core/parent/child_life_controller.ex | 2 +- .../core/parent/child_life_controller/link_utils.ex | 11 +++++------ lib/membrane/pipeline.ex | 5 ++--- test/membrane/integration/child_pad_removed_test.exs | 3 +-- 6 files changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/membrane/bin.ex b/lib/membrane/bin.ex index c7546c256..21f439347 100644 --- a/lib/membrane/bin.ex +++ b/lib/membrane/bin.ex @@ -93,9 +93,8 @@ defmodule Membrane.Bin do @doc """ Callback invoked when a child removes its pad. - Removing child's pad due to return `t:Membrane.Bin.Action.remove_children()` - or `t:Membrane.Bin.Action.remove_link()` action from parent's callback does - not trigger this callback. + Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()` + from `Membrane.Bin` callbacks does not trigger this callback. """ @callback handle_child_pad_removed( child :: Child.name(), diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index 3198b7a7d..5fe2cc474 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -80,8 +80,8 @@ defmodule Membrane.Core.Bin.PadController do state end - @spec remove_dynamic_pad(Pad.ref(), State.t()) :: State.t() - def remove_dynamic_pad(pad_ref, state) do + @spec remove_dynamic_pad!(Pad.ref(), State.t()) :: State.t() + def remove_dynamic_pad!(pad_ref, state) do case pad_ref do Pad.ref(_name, _id) -> Message.send(state.parent_pid, :child_pad_removed, [state.name, pad_ref]) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index f335f7ea3..9ff28b532 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -73,7 +73,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do } @doc """ - Handles `ChildrenSpec` returned with `spec` action. + Handles `Membrane.ChildrenSpec` returned with `spec` action. Handling a spec consists of the following steps: - Parse the spec diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index d3a0827dd..a989db9f6 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -41,7 +41,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do opposite_endpoint(link, child) |> case do %Endpoint{child: {Membrane.Bin, :itself}} = bin_endpoint -> - PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) + PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) @@ -58,7 +58,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) bin_endpoint = opposite_endpoint(link, child_endpoint.child) - state = PadController.remove_dynamic_pad(bin_endpoint.pad_ref, state) + state = PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) delete_link(state, link) else @@ -94,7 +94,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do Enum.reduce(dropped_links, state, fn link, state -> case endpoint_to_unlink(child_name, link) do %Endpoint{child: {Membrane.Bin, :itself}, pad_ref: pad_ref} -> - PadController.remove_dynamic_pad(pad_ref, state) + PadController.remove_dynamic_pad!(pad_ref, state) %Endpoint{} = endpoint -> Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) @@ -187,15 +187,14 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do defp delete_link(state, link) do links = Map.delete(state.links, link.id) state = Map.put(state, :links, links) - spec_ref = link.spec_ref - with %{^spec_ref => spec_data} <- state.pending_specs do + with {:ok, spec_data} <- Map.fetch(state.pending_specs, spec_ref) do new_links_ids = Enum.reject(spec_data.links_ids, &(&1 == link.id)) state = put_in(state, [:pending_specs, spec_ref, :links_ids], new_links_ids) ChildLifeController.proceed_spec_startup(spec_ref, state) else - _pending_specs -> state + :error -> state end end diff --git a/lib/membrane/pipeline.ex b/lib/membrane/pipeline.ex index a897576e3..a81d00146 100644 --- a/lib/membrane/pipeline.ex +++ b/lib/membrane/pipeline.ex @@ -138,9 +138,8 @@ defmodule Membrane.Pipeline do @doc """ Callback invoked when a child removes its pad. - Removing child's pad due to return `t:Membrane.Bin.Action.remove_children()` - or `t:Membrane.Bin.Action.remove_link()` action from parent's callback does - not trigger this callback. + Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()` + from `Membrane.Pipeline` callbacks does not trigger this callback. """ @callback handle_child_pad_removed( child :: Child.name(), diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs index b9afc9c6f..d6e5758c2 100644 --- a/test/membrane/integration/child_pad_removed_test.exs +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -140,7 +140,6 @@ defmodule Membrane.Integration.ChildPadRemovedTest do end assert_receive {:child_pad_removed, :bin, Pad.ref(:output, _id)} - assert_child_exists(pipeline, :bin) assert_child_exists(pipeline, :sink) @@ -194,8 +193,8 @@ defmodule Membrane.Integration.ChildPadRemovedTest do assert_receive {:child_pad_removed, :bin, Pad.ref(:output, _id)} assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, _reason} - assert_child_exists(pipeline, :bin) + Pipeline.terminate(pipeline, blocking?: true) end end From 818ef92bf5efac1de83995f0c99a232600e0ad2d Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 25 Jan 2023 17:35:37 +0100 Subject: [PATCH 08/33] Stop raising, on unliunking static pad, when component is terminating, but it's spec is not ready yet --- lib/membrane/core/bin.ex | 4 ++-- lib/membrane/core/bin/pad_controller.ex | 8 ++++---- lib/membrane/core/element.ex | 4 ++-- lib/membrane/core/element/pad_controller.ex | 6 +++--- .../core/parent/child_life_controller/link_utils.ex | 13 +++++++++---- test/membrane/core/element/pad_controller_test.exs | 4 ++-- test/membrane/core/element_test.exs | 5 ++++- .../membrane/integration/child_pad_removed_test.exs | 6 +++--- 8 files changed, 29 insertions(+), 21 deletions(-) diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index e5950b72a..5b1d59679 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -150,8 +150,8 @@ defmodule Membrane.Core.Bin do @compile {:inline, do_handle_info: 2} - defp do_handle_info(Message.new(:handle_unlink, pad_ref), state) do - state = PadController.handle_unlink(pad_ref, state) + defp do_handle_info(Message.new(:handle_unlink, [pad_ref, mode]), state) do + state = PadController.handle_unlink(pad_ref, mode, state) {:noreply, state} end diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index 5fe2cc474..bf118043f 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -266,15 +266,15 @@ defmodule Membrane.Core.Bin.PadController do @doc """ Handles situation where the pad has been unlinked (e.g. when connected element has been removed from the pipeline) """ - @spec handle_unlink(Pad.ref(), Core.Bin.State.t()) :: Core.Bin.State.t() - def handle_unlink(pad_ref, state) do + @spec handle_unlink(Pad.ref(), :soft | :hard, Core.Bin.State.t()) :: Core.Bin.State.t() + def handle_unlink(pad_ref, mode, state) do with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do state = maybe_handle_pad_removed(pad_ref, state) endpoint = PadModel.get_data!(state, pad_ref, :endpoint) {pad_data, state} = PadModel.pop_data!(state, pad_ref) if endpoint do - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + Message.send(endpoint.pid, :handle_unlink, [endpoint.pad_ref, mode]) ChildLifeController.proceed_spec_startup(pad_data.spec_ref, state) else Membrane.Logger.debug(""" @@ -285,7 +285,7 @@ defmodule Membrane.Core.Bin.PadController do state end else - {:ok, %{availability: :always}} when state.terminating? -> + {:ok, %{availability: :always}} when state.terminating? or mode == :soft -> state {:ok, %{availability: :always}} -> diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index e648041f5..bc23befbb 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -200,8 +200,8 @@ defmodule Membrane.Core.Element do {:noreply, state} end - defp do_handle_info(Message.new(:handle_unlink, pad_ref), state) do - state = PadController.handle_unlink(pad_ref, state) + defp do_handle_info(Message.new(:handle_unlink, [pad_ref, mode]), state) do + state = PadController.handle_unlink(pad_ref, mode, state) {:noreply, state} end diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex index a1ac7e87e..1c84927c3 100644 --- a/lib/membrane/core/element/pad_controller.ex +++ b/lib/membrane/core/element/pad_controller.ex @@ -190,15 +190,15 @@ defmodule Membrane.Core.Element.PadController do Executes `handle_pad_removed` callback if the pad was dynamic. Note: it also flushes all buffers from PlaybackBuffer. """ - @spec handle_unlink(Pad.ref(), State.t()) :: State.t() - def handle_unlink(pad_ref, state) do + @spec handle_unlink(Pad.ref(), :soft | :hard, State.t()) :: State.t() + def handle_unlink(pad_ref, mode, state) do with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do state = generate_eos_if_needed(pad_ref, state) state = maybe_handle_pad_removed(pad_ref, state) state = remove_pad_associations(pad_ref, state) PadModel.delete_data!(state, pad_ref) else - {:ok, %{availability: :always}} when state.terminating? -> + {:ok, %{availability: :always}} when state.terminating? or mode == :soft -> state {:ok, %{availability: :always}} -> diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index a989db9f6..b65e19d52 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -44,7 +44,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + send_handle_unlink(endpoint, state) state end |> delete_link(link) @@ -55,7 +55,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) + send_handle_unlink(child_endpoint, state) bin_endpoint = opposite_endpoint(link, child_endpoint.child) state = PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) @@ -63,7 +63,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do delete_link(state, link) else for endpoint <- [link.from, link.to] do - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + send_handle_unlink(endpoint, state) end delete_link(state, link) @@ -97,7 +97,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad!(pad_ref, state) %Endpoint{} = endpoint -> - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + send_handle_unlink(endpoint, state) state end end) @@ -326,4 +326,9 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end end end + + defp send_handle_unlink(%Endpoint{child: child} = endpoint, state) do + mode = if state.children[child].terminating?, do: :soft, else: :hard + Message.send(endpoint.pid, :handle_unlink, [endpoint.pad_ref, mode]) + end end diff --git a/test/membrane/core/element/pad_controller_test.exs b/test/membrane/core/element/pad_controller_test.exs index 4df843206..cbd5c4738 100644 --- a/test/membrane/core/element/pad_controller_test.exs +++ b/test/membrane/core/element/pad_controller_test.exs @@ -103,7 +103,7 @@ defmodule Membrane.Core.Element.PadControllerTest do assert state.pads_data |> Map.has_key?(:output) assert_raise Membrane.PadError, fn -> - @module.handle_unlink(:output, state) + @module.handle_unlink(:output, :hard, state) end end @@ -111,7 +111,7 @@ defmodule Membrane.Core.Element.PadControllerTest do pad_ref = Pad.ref(:input, 0) state = prepare_dynamic_state(DynamicFilter, :element, :input, pad_ref) assert state.pads_data |> Map.has_key?(pad_ref) - state = @module.handle_unlink(pad_ref, state) + state = @module.handle_unlink(pad_ref, :hard, state) assert state.internal_state[:last_event] == nil assert state.internal_state.last_pad_removed == pad_ref refute state.pads_data |> Map.has_key?(pad_ref) diff --git a/test/membrane/core/element_test.exs b/test/membrane/core/element_test.exs index 549c1604b..8a8a3f0a2 100644 --- a/test/membrane/core/element_test.exs +++ b/test/membrane/core/element_test.exs @@ -255,7 +255,10 @@ defmodule Membrane.Core.ElementTest do test "should handle unlinking pads" do assert {:noreply, state} = - Element.handle_info(Message.new(:handle_unlink, :dynamic_input), linked_state()) + Element.handle_info( + Message.new(:handle_unlink, [:dynamic_input, :hard]), + linked_state() + ) refute Map.has_key?(state.pads_data, :dynamic_input) end diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs index d6e5758c2..304bb4fe6 100644 --- a/test/membrane/integration/child_pad_removed_test.exs +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -128,7 +128,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do # This sleep is executed to ensure, that spec has already been finished. # After fixing bug occuring, when parent returns :remove_link before # spec status is :ready, this sleep will become unneceassary. - Process.sleep(100) + # Process.sleep(100) execute_actions_in_bin(pipeline, bin_actions) receive do @@ -157,7 +157,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do # This sleep is executed to ensure, that spec has already been finished. # After fixing bug occuring, when parent returns :remove_link before # spec status is :ready, this sleep will become unneceassary. - Process.sleep(100) + # Process.sleep(100) sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) monitor_ref = Process.monitor(sink_pid) @@ -184,7 +184,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do # This sleep is executed to ensure, that spec has already been finished. # After fixing bug occuring, when parent returns :remove_link before # spec status is :ready, this sleep will become unneceassary. - Process.sleep(100) + # Process.sleep(100) sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) monitor_ref = Process.monitor(sink_pid) From 78609063e2afa200a6c659bbb18459f65f5da94b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 25 Jan 2023 18:19:44 +0100 Subject: [PATCH 09/33] Remove leftovers from handle_child_pad_removed tests --- .../integration/child_pad_removed_test.exs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs index 304bb4fe6..83665c0c0 100644 --- a/test/membrane/integration/child_pad_removed_test.exs +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -1,6 +1,8 @@ defmodule Membrane.Integration.ChildPadRemovedTest do use ExUnit.Case, async: false + import Membrane.Testing.Assertions + alias Membrane.Testing require Membrane.Pad, as: Pad @@ -15,6 +17,12 @@ defmodule Membrane.Integration.ChildPadRemovedTest do def_input_pad :input, flow_control: :push, availability: :on_request, accepted_format: _any def_options test_process: [spec: pid()] + @impl true + def handle_init(ctx, opts) do + send(opts.test_process, {:init, ctx.name}) + {[], Map.from_struct(opts)} + end + @impl true def handle_pad_added(_pad, ctx, state) do send(state.test_process, {:pad_added, ctx.name}) @@ -32,6 +40,12 @@ defmodule Membrane.Integration.ChildPadRemovedTest do use Membrane.Sink def_input_pad :input, flow_control: :push, accepted_format: _any def_options test_process: [spec: pid()] + + @impl true + def handle_init(ctx, opts) do + send(opts.test_process, {:init, ctx.name}) + {[], Map.from_struct(opts)} + end end defmodule DynamicBin do @@ -125,10 +139,6 @@ defmodule Membrane.Integration.ChildPadRemovedTest do ] do pipeline = start_link_pipeline!(DynamicBin, DynamicSink) - # This sleep is executed to ensure, that spec has already been finished. - # After fixing bug occuring, when parent returns :remove_link before - # spec status is :ready, this sleep will become unneceassary. - # Process.sleep(100) execute_actions_in_bin(pipeline, bin_actions) receive do @@ -154,11 +164,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do ] do pipeline = start_pipeline!(DynamicBin, StaticSink) - # This sleep is executed to ensure, that spec has already been finished. - # After fixing bug occuring, when parent returns :remove_link before - # spec status is :ready, this sleep will become unneceassary. - # Process.sleep(100) - + assert_receive {:init, :sink} sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) monitor_ref = Process.monitor(sink_pid) @@ -181,11 +187,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do ] do pipeline = start_link_pipeline!(DynamicBin, StaticSink, remove_children: :sink) - # This sleep is executed to ensure, that spec has already been finished. - # After fixing bug occuring, when parent returns :remove_link before - # spec status is :ready, this sleep will become unneceassary. - # Process.sleep(100) - + assert_receive {:init, :sink} sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) monitor_ref = Process.monitor(sink_pid) From f3461614b46ea1286e0a58b9e406e6f58002b01c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 26 Jan 2023 16:42:36 +0100 Subject: [PATCH 10/33] Make dependent specs a map --- .../core/parent/child_life_controller.ex | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 9ff28b532..b3196453d 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -34,7 +34,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do children_names: [Child.name()], links_ids: [Link.id()], awaiting_responses: MapSet.t({Link.id(), Membrane.Pad.direction()}), - dependent_specs: MapSet.t(spec_ref) + dependent_specs: %{spec_ref() => [Child.name()]} } @type pending_specs :: %{spec_ref() => pending_spec()} @@ -145,12 +145,19 @@ defmodule Membrane.Core.Parent.ChildLifeController do state = %{state | links: Map.merge(state.links, Map.new(resolved_links, &{&1.id, &1}))} dependent_specs = + # resolved_links + # |> Enum.flat_map(&[&1.from.child_spec_ref, &1.to.child_spec_ref]) + # |> Enum.filter(fn spec_ref -> + # get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses + # end) + # |> MapSet.new() resolved_links - |> Enum.flat_map(&[&1.from.child_spec_ref, &1.to.child_spec_ref]) - |> Enum.filter(fn spec_ref -> + |> Enum.flat_map(&[&1.from, &1.to]) + |> Enum.map(& {&1.child_spec_ref, &1.child}) + |> Enum.filter(fn {spec_ref, _child} -> get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses end) - |> MapSet.new() + |> Enum.group_by(fn {spec_ref, _child} -> spec_ref end) state = put_in(state, [:pending_specs, spec_ref], %{ @@ -346,7 +353,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do defp do_proceed_spec_startup(spec_ref, %{status: :initializing} = spec_data, state) do Membrane.Logger.debug( - "Proceeding spec #{inspect(spec_ref)} startup: initializing, dependent specs: #{inspect(spec_data.dependent_specs)}" + "Proceeding spec #{inspect(spec_ref)} startup: initializing, dependent specs: #{inspect(Map.keys(spec_data.dependent_specs))}" ) %{children: children} = state @@ -642,9 +649,9 @@ defmodule Membrane.Core.Parent.ChildLifeController do defp remove_spec_from_dependencies(spec_ref, state) do dependent_specs = state.pending_specs - |> Enum.filter(fn {_ref, data} -> spec_ref in data.dependent_specs end) + |> Enum.filter(fn {_ref, data} -> Map.has_key?(data.dependent_specs, spec_ref) end) |> Map.new(fn {ref, data} -> - {ref, Map.update!(data, :dependent_specs, &MapSet.delete(&1, spec_ref))} + {ref, Map.update!(data, :dependent_specs, &Map.delete(&1, spec_ref))} end) state = %{state | pending_specs: Map.merge(state.pending_specs, dependent_specs)} From 270fb9da8664174b59b77e4c170ca7327f864e75 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 26 Jan 2023 18:31:35 +0100 Subject: [PATCH 11/33] Remove link from specs, when child removes it's pad --- lib/membrane/core/element/pad_controller.ex | 1 + .../core/parent/child_life_controller.ex | 123 ++++++++++++++++-- .../child_life_controller/link_utils.ex | 59 +++++---- .../integration/child_pad_removed_test.exs | 2 - 4 files changed, 147 insertions(+), 38 deletions(-) diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex index 1c84927c3..c6d950d41 100644 --- a/lib/membrane/core/element/pad_controller.ex +++ b/lib/membrane/core/element/pad_controller.ex @@ -207,6 +207,7 @@ defmodule Membrane.Core.Element.PadController do {:error, :unknown_pad} -> with false <- state.terminating?, + :hard <- mode, %{availability: :always} <- state.pads_info[Pad.name_by_ref(pad_ref)] do raise Membrane.PadError, "Tried to unlink a static pad #{inspect(pad_ref)}, before it was linked. Static pads cannot be unlinked unless element is terminating" diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index b3196453d..9dcf6b3e7 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -144,16 +144,16 @@ defmodule Membrane.Core.Parent.ChildLifeController do resolved_links = LinkUtils.resolve_links(links, spec_ref, state) state = %{state | links: Map.merge(state.links, Map.new(resolved_links, &{&1.id, &1}))} + # resolved_links + # |> Enum.flat_map(&[&1.from.child_spec_ref, &1.to.child_spec_ref]) + # |> Enum.filter(fn spec_ref -> + # get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses + # end) + # |> MapSet.new() dependent_specs = - # resolved_links - # |> Enum.flat_map(&[&1.from.child_spec_ref, &1.to.child_spec_ref]) - # |> Enum.filter(fn spec_ref -> - # get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses - # end) - # |> MapSet.new() resolved_links |> Enum.flat_map(&[&1.from, &1.to]) - |> Enum.map(& {&1.child_spec_ref, &1.child}) + |> Enum.map(&{&1.child_spec_ref, &1.child}) |> Enum.filter(fn {spec_ref, _child} -> get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses end) @@ -494,10 +494,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end @spec handle_remove_children( - Child.ref() - | [Child.ref()] - | Child.group() - | [Child.group()], + Child.ref() | [Child.ref()] | Child.group() | [Child.group()], Parent.state() ) :: Parent.state() def handle_remove_children(children_or_children_groups, state) do @@ -539,6 +536,110 @@ defmodule Membrane.Core.Parent.ChildLifeController do LinkUtils.remove_link(child_name, pad_ref, state) end + defp remove_children_from_specs(children, state) do + children = Bunch.listify(children) + children_set = MapSet.new(children) + + children_links_ids_set = + state.links + |> Enum.filter(&(&1.from.child in children_set or &1.to.child in children_set)) + |> MapSet.new(& &1.id) + + affected_specs = + state.pending_specs + |> Enum.filter(fn {_ref, spec_data} -> + Enum.any?(spec_data.children, &(&1 in children_set)) or + Enum.any?(spec_data.links, &(&1 in children_links_ids_set)) + end) + + updated_specs = + affected_specs + |> Map.new(fn {spec_ref, spec_data} -> + children_names = + spec_data.children_names + |> Enum.reject(&(&1 in children_set)) + + links_ids = Enum.reject(spec_data.links_ids, &(&1 in children_links_ids_set)) + + awaiting_responses = + spec_data.awaiting_responses + |> Enum.reject(fn {link_id, _direction} -> link_id in children_links_ids_set end) + |> MapSet.new() + + dependent_specs = + spec_data.dependent_specs + |> Enum.map(fn {ref, spec_children} -> + {ref, Enum.reject(spec_children, &(&1 in children_set))} + end) + |> Enum.reject(&match?({_ref, []}, &1)) + |> Map.new() + + spec_data = %{ + spec_data + | children_names: children_names, + links_ids: links_ids, + awaiting_responses: awaiting_responses, + dependent_specs: dependent_specs + } + + {spec_ref, spec_data} + end) + + state = Map.update!(state, :pending_specs, &Map.merge(&1, updated_specs)) + + Enum.reduce(updated_specs, state, fn {spec_ref, _spec_data}, state -> + proceed_spec_startup(spec_ref, state) + end) + end + + @spec remove_link_from_spec(Link.id(), Parent.state()) :: Parent.state() + def remove_link_from_spec(link_id, state) do + {:ok, removed_link} = Map.fetch(state.links, link_id) + spec_ref = removed_link.spec_ref + + with {:ok, spec_data} <- Map.fetch(state.pending_specs, spec_ref) do + links_ids = Enum.reject(spec_data.links_ids, &(&1 == link_id)) + + spec_links_endpoints = + Enum.flat_map(links_ids, fn id -> + link = state.links[id] + [link.from.child, link.to.child] + end) + + dependent_specs = + [removed_link.from.child, removed_link.to.child] + |> Enum.filter(&(&1 not in spec_links_endpoints)) + |> case do + [] -> + spec_data.dependent_specs + + endpoints_to_remove -> + spec_data.dependent_specs + |> Enum.map(fn {spec_ref, spec_children} -> + {spec_ref, Enum.reject(spec_children, &(&1 in endpoints_to_remove))} + end) + |> Enum.reject(&match?({_ref, []}, &1)) + |> Map.new() + end + + awaiting_responses = + spec_data.awaiting_responses + |> MapSet.difference(MapSet.new([{link_id, :input}, {link_id, :output}])) + + spec_data = %{ + spec_data + | dependent_specs: dependent_specs, + links_ids: links_ids, + awaiting_responses: awaiting_responses + } + + state = put_in(state, [:pending_specs, spec_ref], spec_data) + proceed_spec_startup(spec_ref, state) + else + :error -> state + end + end + @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do # TODO: when spec is not ready yet, delete specific link from it and trigger proceeding diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index b65e19d52..85b49c1d1 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -38,36 +38,42 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do def handle_child_pad_removed(child, pad, state) do {:ok, link} = get_link(state.links, child, pad) - opposite_endpoint(link, child) - |> case do - %Endpoint{child: {Membrane.Bin, :itself}} = bin_endpoint -> - PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) + state = + opposite_endpoint(link, child) + |> case do + %Endpoint{child: {Membrane.Bin, :itself}} = bin_endpoint -> + PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) - %Endpoint{} = endpoint -> - send_handle_unlink(endpoint, state) - state - end - |> delete_link(link) + %Endpoint{} = endpoint -> + send_handle_unlink(endpoint, state) + state + end + + state = ChildLifeController.remove_link_from_spec(link.id, state) + + delete_link(link, state) end @spec remove_link(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def remove_link(child_name, pad_ref, state) do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do - if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do - child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - send_handle_unlink(child_endpoint, state) + state = + if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do + child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) + send_handle_unlink(child_endpoint, state) + + bin_endpoint = opposite_endpoint(link, child_endpoint.child) + PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) + else + for endpoint <- [link.from, link.to] do + send_handle_unlink(endpoint, state) + end - bin_endpoint = opposite_endpoint(link, child_endpoint.child) - state = PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) - - delete_link(state, link) - else - for endpoint <- [link.from, link.to] do - send_handle_unlink(endpoint, state) + state end - delete_link(state, link) - end + state = ChildLifeController.remove_link_from_spec(link.id, state) + delete_link(link, state) else {:error, :not_found} -> with %{^child_name => _child_entry} <- state.children do @@ -184,9 +190,8 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do defp opposite_endpoint(%Link{to: %Endpoint{child: child}, from: from}, child), do: from - defp delete_link(state, link) do - links = Map.delete(state.links, link.id) - state = Map.put(state, :links, links) + defp delete_link(link, state) do + {_link, state} = pop_in(state, [:links, link.id]) spec_ref = link.spec_ref with {:ok, spec_data} <- Map.fetch(state.pending_specs, spec_ref) do @@ -328,7 +333,11 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end defp send_handle_unlink(%Endpoint{child: child} = endpoint, state) do - mode = if state.children[child].terminating?, do: :soft, else: :hard + mode = + if state.children[child].terminating?, + do: :soft, + else: :hard + Message.send(endpoint.pid, :handle_unlink, [endpoint.pad_ref, mode]) end end diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs index 83665c0c0..86d8b00bd 100644 --- a/test/membrane/integration/child_pad_removed_test.exs +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -1,8 +1,6 @@ defmodule Membrane.Integration.ChildPadRemovedTest do use ExUnit.Case, async: false - import Membrane.Testing.Assertions - alias Membrane.Testing require Membrane.Pad, as: Pad From c1fd3bc92e85647c896d87d794f05737360f9bfc Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 27 Jan 2023 14:30:34 +0100 Subject: [PATCH 12/33] Update specs, when parent removes a child --- .../core/parent/child_life_controller.ex | 26 +++++--- test/membrane/integration/linking_test.exs | 59 +++++++++++++++++++ 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 9dcf6b3e7..ab1df0fbb 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -463,11 +463,13 @@ defmodule Membrane.Core.Parent.ChildLifeController do case Map.fetch(state.links, link_id) do {:ok, %Link{spec_ref: spec_ref}} -> state = - update_in( - state, - [:pending_specs, spec_ref, :awaiting_responses], - &MapSet.delete(&1, {link_id, direction}) - ) + with %{pending_specs: %{^spec_ref => _spec_data}} <- state do + update_in( + state, + [:pending_specs, spec_ref, :awaiting_responses], + &MapSet.delete(&1, {link_id, direction}) + ) + end proceed_spec_startup(spec_ref, state) @@ -526,7 +528,13 @@ defmodule Membrane.Core.Parent.ChildLifeController do """) end - data |> Enum.filter(& &1.ready?) |> Enum.each(&Message.send(&1.pid, :terminate)) + # data |> Enum.filter(& &1.ready?) |> Enum.each(&Message.send(&1.pid, :terminate)) + + Enum.each(data, &Message.send(&1.pid, :terminate)) + + children_names = Enum.map(data, & &1.name) + state = remove_children_from_specs(children_names, state) + Parent.ChildrenModel.update_children!(state, refs, &%{&1 | terminating?: true}) end @@ -541,15 +549,15 @@ defmodule Membrane.Core.Parent.ChildLifeController do children_set = MapSet.new(children) children_links_ids_set = - state.links + Map.values(state.links) |> Enum.filter(&(&1.from.child in children_set or &1.to.child in children_set)) |> MapSet.new(& &1.id) affected_specs = state.pending_specs |> Enum.filter(fn {_ref, spec_data} -> - Enum.any?(spec_data.children, &(&1 in children_set)) or - Enum.any?(spec_data.links, &(&1 in children_links_ids_set)) + Enum.any?(spec_data.children_names, &(&1 in children_set)) or + Enum.any?(spec_data.links_ids, &(&1 in children_links_ids_set)) end) updated_specs = diff --git a/test/membrane/integration/linking_test.exs b/test/membrane/integration/linking_test.exs index 9c59fda24..a68962dbb 100644 --- a/test/membrane/integration/linking_test.exs +++ b/test/membrane/integration/linking_test.exs @@ -503,6 +503,65 @@ defmodule Membrane.Integration.LinkingTest do end end + describe "Spec shouldn't wait on links with" do + defmodule LazyBin do + use Membrane.Bin + + def_output_pad :output, + availability: :on_request, + accepted_format: _any + end + + defmodule Sink do + use Membrane.Sink + + def_input_pad :input, + accepted_format: _any + + @impl true + def handle_init(_ctx, _opts) do + {[notify_parent: :init], %{}} + end + end + + test "removed child" do + spec = child(:bin, LazyBin) |> child(:sink, Sink) + pipeline = Testing.Pipeline.start_link_supervised!(spec: spec) + + assert_pipeline_notified(pipeline, :sink, :init) + sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) + monitor_ref = Process.monitor(sink_pid) + + Testing.Pipeline.execute_actions(pipeline, remove_children: :sink) + + assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, :normal} + assert_pipeline_play(pipeline) + + Testing.Pipeline.terminate(pipeline, blocking?: true) + end + + @tag :dupa + test "crashed child" do + spec = [ + {child(:sink, Sink), group: :group, crash_group_mode: :temporary}, + child(:bin, LazyBin) |> get_child(:sink) + ] + + pipeline = Testing.Pipeline.start_link_supervised!(spec: spec) + + assert_pipeline_notified(pipeline, :sink, :init) + sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) + monitor_ref = Process.monitor(sink_pid) + + Process.exit(sink_pid, :kill) + + assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, :kill} + assert_pipeline_play(pipeline) + + Testing.Pipeline.terminate(pipeline, blocking?: true) + end + end + defp assert_link_removed(pipeline, id) do assert_pipeline_notified(pipeline, :source, {:handle_pad_removed, Pad.ref(:output, ^id)}) assert_pipeline_notified(pipeline, :sink, {:handle_pad_removed, Pad.ref(:input, ^id)}) From b111be3e6c5046f350bd371cc1ceacdc75e6807a Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 27 Jan 2023 15:15:02 +0100 Subject: [PATCH 13/33] Fix bug in Testing.Pipeline --- lib/membrane/testing/pipeline.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/membrane/testing/pipeline.ex b/lib/membrane/testing/pipeline.ex index 7d56c094b..8e2163c35 100644 --- a/lib/membrane/testing/pipeline.ex +++ b/lib/membrane/testing/pipeline.ex @@ -482,8 +482,8 @@ defmodule Membrane.Testing.Pipeline do defp eval_injected_module_callback(callback, args, state) - defp eval_injected_module_callback(_callback, _args, %State{module: nil} = state), - do: {[], state} + defp eval_injected_module_callback(_callback, _args, %State{module: nil}), + do: {[], nil} defp eval_injected_module_callback(callback, args, state) do apply(state.module, callback, args ++ [state.custom_pipeline_state]) From 8b53257bdab9fee91a44a73b9fa2cf4d000c43b7 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 30 Jan 2023 13:24:48 +0100 Subject: [PATCH 14/33] Fix bug in pipeline termination & write tests for killing elements while specs are processed --- .../core/parent/child_life_controller.ex | 26 ++++++------------ test/membrane/integration/linking_test.exs | 27 +++++++++++-------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index ab1df0fbb..e2c32c6db 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -144,12 +144,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do resolved_links = LinkUtils.resolve_links(links, spec_ref, state) state = %{state | links: Map.merge(state.links, Map.new(resolved_links, &{&1.id, &1}))} - # resolved_links - # |> Enum.flat_map(&[&1.from.child_spec_ref, &1.to.child_spec_ref]) - # |> Enum.filter(fn spec_ref -> - # get_in(state, [:pending_specs, spec_ref, :status]) in @spec_dependency_requiring_statuses - # end) - # |> MapSet.new() dependent_specs = resolved_links |> Enum.flat_map(&[&1.from, &1.to]) @@ -528,8 +522,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do """) end - # data |> Enum.filter(& &1.ready?) |> Enum.each(&Message.send(&1.pid, :terminate)) - Enum.each(data, &Message.send(&1.pid, :terminate)) children_names = Enum.map(data, & &1.name) @@ -699,21 +691,18 @@ defmodule Membrane.Core.Parent.ChildLifeController do %{pid: child_pid} = ChildrenModel.get_child_data!(state, child_name) with {:ok, group} <- CrashGroupUtils.get_group_by_member_pid(child_pid, state) do + existing_members = + group.members + |> Enum.filter(&Map.has_key?(state.children, &1)) + + state = remove_children_from_specs(existing_members, state) + state = Enum.reduce(existing_members, state, &LinkUtils.unlink_element/2) + {result, state} = crash_all_group_members(group, child_name, state) |> remove_child_from_crash_group(group, child_pid) if result == :removed do - state = - Enum.reduce(group.members, state, fn child_name, state -> - with {%{spec_ref: spec_ref}, state} <- pop_in(state, [:children, child_name]) do - state = LinkUtils.unlink_element(child_name, state) - cleanup_spec_startup(spec_ref, state) - else - {nil, state} -> state - end - end) - exec_handle_crash_group_down_callback( group.name, group.members, @@ -723,6 +712,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do else state end + |> Bunch.Access.delete_in([:children, child_name]) else {:error, :not_member} when reason == {:shutdown, :membrane_crash_group_kill} -> raise Membrane.PipelineError, diff --git a/test/membrane/integration/linking_test.exs b/test/membrane/integration/linking_test.exs index a68962dbb..7985cfbed 100644 --- a/test/membrane/integration/linking_test.exs +++ b/test/membrane/integration/linking_test.exs @@ -510,6 +510,11 @@ defmodule Membrane.Integration.LinkingTest do def_output_pad :output, availability: :on_request, accepted_format: _any + + @impl true + def handle_playing(_ctx, state) do + {[notify_parent: :playing], state} + end end defmodule Sink do @@ -535,30 +540,30 @@ defmodule Membrane.Integration.LinkingTest do Testing.Pipeline.execute_actions(pipeline, remove_children: :sink) assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, :normal} - assert_pipeline_play(pipeline) + assert_pipeline_notified(pipeline, :bin, :playing) - Testing.Pipeline.terminate(pipeline, blocking?: true) + assert :ok == Testing.Pipeline.terminate(pipeline, blocking?: true) end - @tag :dupa test "crashed child" do + sink_ref = Child.ref(:sink, group: :group) + spec = [ {child(:sink, Sink), group: :group, crash_group_mode: :temporary}, - child(:bin, LazyBin) |> get_child(:sink) + child(:bin, LazyBin) |> get_child(sink_ref) ] pipeline = Testing.Pipeline.start_link_supervised!(spec: spec) - assert_pipeline_notified(pipeline, :sink, :init) - sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) - monitor_ref = Process.monitor(sink_pid) + assert_pipeline_notified(pipeline, sink_ref, :init) - Process.exit(sink_pid, :kill) + Testing.Pipeline.get_child_pid!(pipeline, sink_ref) + |> Process.exit(:kill) - assert_receive {:DOWN, ^monitor_ref, :process, ^sink_pid, :kill} - assert_pipeline_play(pipeline) + assert_pipeline_crash_group_down(pipeline, :group) + assert_pipeline_notified(pipeline, :bin, :playing) - Testing.Pipeline.terminate(pipeline, blocking?: true) + assert :ok == Testing.Pipeline.terminate(pipeline, blocking?: true) end end From c2112be1333629f30524b116fbddbaef70cfb423 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 30 Jan 2023 14:14:01 +0100 Subject: [PATCH 15/33] Refactor :handle_unlink message --- lib/membrane/core/bin.ex | 4 ++-- lib/membrane/core/bin/pad_controller.ex | 8 ++++---- lib/membrane/core/element.ex | 4 ++-- lib/membrane/core/element/pad_controller.ex | 7 +++---- .../parent/child_life_controller/link_utils.ex | 17 ++++------------- .../core/element/pad_controller_test.exs | 4 ++-- test/membrane/core/element_test.exs | 2 +- 7 files changed, 18 insertions(+), 28 deletions(-) diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index 5b1d59679..e5950b72a 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -150,8 +150,8 @@ defmodule Membrane.Core.Bin do @compile {:inline, do_handle_info: 2} - defp do_handle_info(Message.new(:handle_unlink, [pad_ref, mode]), state) do - state = PadController.handle_unlink(pad_ref, mode, state) + defp do_handle_info(Message.new(:handle_unlink, pad_ref), state) do + state = PadController.handle_unlink(pad_ref, state) {:noreply, state} end diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index bf118043f..5fe2cc474 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -266,15 +266,15 @@ defmodule Membrane.Core.Bin.PadController do @doc """ Handles situation where the pad has been unlinked (e.g. when connected element has been removed from the pipeline) """ - @spec handle_unlink(Pad.ref(), :soft | :hard, Core.Bin.State.t()) :: Core.Bin.State.t() - def handle_unlink(pad_ref, mode, state) do + @spec handle_unlink(Pad.ref(), Core.Bin.State.t()) :: Core.Bin.State.t() + def handle_unlink(pad_ref, state) do with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do state = maybe_handle_pad_removed(pad_ref, state) endpoint = PadModel.get_data!(state, pad_ref, :endpoint) {pad_data, state} = PadModel.pop_data!(state, pad_ref) if endpoint do - Message.send(endpoint.pid, :handle_unlink, [endpoint.pad_ref, mode]) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) ChildLifeController.proceed_spec_startup(pad_data.spec_ref, state) else Membrane.Logger.debug(""" @@ -285,7 +285,7 @@ defmodule Membrane.Core.Bin.PadController do state end else - {:ok, %{availability: :always}} when state.terminating? or mode == :soft -> + {:ok, %{availability: :always}} when state.terminating? -> state {:ok, %{availability: :always}} -> diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index bc23befbb..e648041f5 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -200,8 +200,8 @@ defmodule Membrane.Core.Element do {:noreply, state} end - defp do_handle_info(Message.new(:handle_unlink, [pad_ref, mode]), state) do - state = PadController.handle_unlink(pad_ref, mode, state) + defp do_handle_info(Message.new(:handle_unlink, pad_ref), state) do + state = PadController.handle_unlink(pad_ref, state) {:noreply, state} end diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex index c6d950d41..a1ac7e87e 100644 --- a/lib/membrane/core/element/pad_controller.ex +++ b/lib/membrane/core/element/pad_controller.ex @@ -190,15 +190,15 @@ defmodule Membrane.Core.Element.PadController do Executes `handle_pad_removed` callback if the pad was dynamic. Note: it also flushes all buffers from PlaybackBuffer. """ - @spec handle_unlink(Pad.ref(), :soft | :hard, State.t()) :: State.t() - def handle_unlink(pad_ref, mode, state) do + @spec handle_unlink(Pad.ref(), State.t()) :: State.t() + def handle_unlink(pad_ref, state) do with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do state = generate_eos_if_needed(pad_ref, state) state = maybe_handle_pad_removed(pad_ref, state) state = remove_pad_associations(pad_ref, state) PadModel.delete_data!(state, pad_ref) else - {:ok, %{availability: :always}} when state.terminating? or mode == :soft -> + {:ok, %{availability: :always}} when state.terminating? -> state {:ok, %{availability: :always}} -> @@ -207,7 +207,6 @@ defmodule Membrane.Core.Element.PadController do {:error, :unknown_pad} -> with false <- state.terminating?, - :hard <- mode, %{availability: :always} <- state.pads_info[Pad.name_by_ref(pad_ref)] do raise Membrane.PadError, "Tried to unlink a static pad #{inspect(pad_ref)}, before it was linked. Static pads cannot be unlinked unless element is terminating" diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 85b49c1d1..1a6a2e14f 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -45,7 +45,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> - send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) state end @@ -60,13 +60,13 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do state = if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - send_handle_unlink(child_endpoint, state) + Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) bin_endpoint = opposite_endpoint(link, child_endpoint.child) PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) else for endpoint <- [link.from, link.to] do - send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) end state @@ -103,7 +103,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do PadController.remove_dynamic_pad!(pad_ref, state) %Endpoint{} = endpoint -> - send_handle_unlink(endpoint, state) + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) state end end) @@ -331,13 +331,4 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end end end - - defp send_handle_unlink(%Endpoint{child: child} = endpoint, state) do - mode = - if state.children[child].terminating?, - do: :soft, - else: :hard - - Message.send(endpoint.pid, :handle_unlink, [endpoint.pad_ref, mode]) - end end diff --git a/test/membrane/core/element/pad_controller_test.exs b/test/membrane/core/element/pad_controller_test.exs index cbd5c4738..4df843206 100644 --- a/test/membrane/core/element/pad_controller_test.exs +++ b/test/membrane/core/element/pad_controller_test.exs @@ -103,7 +103,7 @@ defmodule Membrane.Core.Element.PadControllerTest do assert state.pads_data |> Map.has_key?(:output) assert_raise Membrane.PadError, fn -> - @module.handle_unlink(:output, :hard, state) + @module.handle_unlink(:output, state) end end @@ -111,7 +111,7 @@ defmodule Membrane.Core.Element.PadControllerTest do pad_ref = Pad.ref(:input, 0) state = prepare_dynamic_state(DynamicFilter, :element, :input, pad_ref) assert state.pads_data |> Map.has_key?(pad_ref) - state = @module.handle_unlink(pad_ref, :hard, state) + state = @module.handle_unlink(pad_ref, state) assert state.internal_state[:last_event] == nil assert state.internal_state.last_pad_removed == pad_ref refute state.pads_data |> Map.has_key?(pad_ref) diff --git a/test/membrane/core/element_test.exs b/test/membrane/core/element_test.exs index 8a8a3f0a2..a4ef0ad7f 100644 --- a/test/membrane/core/element_test.exs +++ b/test/membrane/core/element_test.exs @@ -256,7 +256,7 @@ defmodule Membrane.Core.ElementTest do test "should handle unlinking pads" do assert {:noreply, state} = Element.handle_info( - Message.new(:handle_unlink, [:dynamic_input, :hard]), + Message.new(:handle_unlink, :dynamic_input), linked_state() ) From 999c59f52f3a47887b5c69f45409d91ba6ba32f5 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 14:32:08 +0100 Subject: [PATCH 16/33] Implement hanlde_child_pad_removed in Testing.Pipeline --- lib/membrane/exceptions.ex | 18 ------ lib/membrane/testing/assertions.ex | 17 ++++++ lib/membrane/testing/pipeline.ex | 40 +++++++++++-- test/membrane/testing/pipeline_test.exs | 76 +++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 22 deletions(-) diff --git a/lib/membrane/exceptions.ex b/lib/membrane/exceptions.ex index 9dd42e6a1..df28edb6f 100644 --- a/lib/membrane/exceptions.ex +++ b/lib/membrane/exceptions.ex @@ -156,21 +156,3 @@ defmodule Membrane.PadDirectionError do } end end - -defmodule Membrane.ChildPadRemovedError do - defexception [:message] - - @impl true - def exception(opts) do - child = Keyword.fetch!(opts, :child) - pad = Keyword.fetch!(opts, :pad) - module = Keyword.fetch!(opts, :module) - - msg = """ - Child #{inspect(child)} removed pad #{inspect(pad)}, but `handle_child_pad_removed/4` - is not implememnted in parent's module (#{inspect(module)}) - """ - - %__MODULE__{message: msg} - end -end diff --git a/lib/membrane/testing/assertions.ex b/lib/membrane/testing/assertions.ex index 1da52d45e..936fc71a0 100644 --- a/lib/membrane/testing/assertions.ex +++ b/lib/membrane/testing/assertions.ex @@ -449,6 +449,23 @@ defmodule Membrane.Testing.Assertions do ) end + @doc """ + Asserts that `Membrane.Testing.Pipeline` child with name `child` removed or is going to + remove it's pad with ref `pad` withing the `timeout` period specified in miliseconds. + """ + defmacro assert_child_pad_removed( + pipeline, + child, + pad, + timeout \\ @default_timeout + ) do + assert_receive_from_pipeline( + pipeline, + {:handle_child_pad_removed, {child, pad}}, + timeout + ) + end + @doc """ Asserts that a cleanup function was registered in `Membrane.Testing.MockResourceGuard`. """ diff --git a/lib/membrane/testing/pipeline.ex b/lib/membrane/testing/pipeline.ex index 8e2163c35..172346a6c 100644 --- a/lib/membrane/testing/pipeline.ex +++ b/lib/membrane/testing/pipeline.ex @@ -92,7 +92,7 @@ defmodule Membrane.Testing.Pipeline do @moduledoc false @enforce_keys [:test_process, :module] - defstruct @enforce_keys ++ [:custom_pipeline_state] + defstruct @enforce_keys ++ [:custom_pipeline_state, :raise_on_child_pad_removed?] @typedoc """ Structure for holding state @@ -109,7 +109,8 @@ defmodule Membrane.Testing.Pipeline do @type t :: %__MODULE__{ test_process: pid() | nil, module: module() | nil, - custom_pipeline_state: Pipeline.state() + custom_pipeline_state: Pipeline.state(), + raise_on_child_pad_removed?: boolean() | nil } end @@ -118,7 +119,8 @@ defmodule Membrane.Testing.Pipeline do module: :default, spec: [ChildrenSpec.builder()], test_process: pid(), - name: Pipeline.name() + name: Pipeline.name(), + raise_on_child_pad_removed?: boolean() ] | [ module: module(), @@ -285,8 +287,15 @@ defmodule Membrane.Testing.Pipeline do case Keyword.get(options, :module, :default) do :default -> spec = Bunch.listify(Keyword.get(options, :spec, [])) + test_process = Keyword.fetch!(options, :test_process) + raise? = Keyword.get(options, :raise_on_child_pad_removed?, true) + + new_state = %State{ + test_process: test_process, + raise_on_child_pad_removed?: raise?, + module: nil + } - new_state = %State{test_process: Keyword.fetch!(options, :test_process), module: nil} {[spec: spec], new_state} module when is_atom(module) -> @@ -466,6 +475,29 @@ defmodule Membrane.Testing.Pipeline do {custom_actions, Map.put(state, :custom_pipeline_state, custom_state)} end + @impl true + def handle_child_pad_removed(child, pad, ctx, state) do + if state.raise_on_child_pad_removed? do + raise """ + Child #{inspect(child)} removed it's pad #{inspect(pad)}. If you want to + handle such a scenario, pass `raise_on_child_pad_removed?: true` option to + `Membrane.Testing.Pipeline.start_*/2` or pass there a pipeline module + implementing this callback via `:name` option. + """ + end + + {custom_actions, custom_state} = + eval_injected_module_callback( + :handle_child_pad_removed, + [child, pad, ctx], + state + ) + + :ok = notify_test_process(state.test_process, {:handle_child_pad_removed, {child, pad}}) + + {custom_actions, Map.put(state, :custom_pipeline_state, custom_state)} + end + @impl true def handle_crash_group_down(group_name, ctx, state) do {custom_actions, custom_state} = diff --git a/test/membrane/testing/pipeline_test.exs b/test/membrane/testing/pipeline_test.exs index 7b2c92307..1939a4920 100644 --- a/test/membrane/testing/pipeline_test.exs +++ b/test/membrane/testing/pipeline_test.exs @@ -169,4 +169,80 @@ defmodule Membrane.Testing.PipelineTest do assert {:error, :pipeline_not_alive} = Pipeline.get_child_pid(pipeline, :bin_1) end + + describe "Testing.Pipeline on handle_child_pad_removed" do + defmodule DynamicElement do + use Membrane.Endpoint + + def_input_pad :input, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + def_output_pad :output, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + @impl true + def handle_pad_added(pad, _ctx, state) do + {[notify_parent: {:pad_added, pad}], state} + end + end + + defmodule Bin do + use Membrane.Bin + + alias Membrane.Testing.PipelineTest.DynamicElement + + require Membrane.Pad, as: Pad + + def_output_pad :output, + accepted_format: _any, + availability: :on_request + + @impl true + def handle_pad_added(pad, _ctx, state) do + spec = + child(:element, DynamicElement) + |> via_out(Pad.ref(:output, 1)) + |> bin_output(pad) + + {[spec: spec], state} + end + + @impl true + def handle_parent_notification(:remove_link, _ctx, state) do + {[remove_link: {:element, Pad.ref(:output, 1)}], state} + end + end + + test "raises with option `:raise_on_child_pad_removed?` set to default" do + spec = + child(:bin, Bin) + |> child(:sink, DynamicElement) + + pipeline = Pipeline.start_supervised!(spec: spec) + monitor_ref = Process.monitor(pipeline) + + assert_pipeline_notified(pipeline, :sink, {:pad_added, _pad}) + Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_link}) + + assert_receive {:DOWN, ^monitor_ref, :process, _pid, _reason} + end + + test "doesn't raise with option `raise_on_child_pad_removed?: false`" do + spec = + child(:bin, Bin) + |> child(:sink, DynamicElement) + + pipeline = Pipeline.start_supervised!(spec: spec, raise_on_child_pad_removed?: false) + monitor_ref = Process.monitor(pipeline) + + assert_pipeline_notified(pipeline, :sink, {:pad_added, _pad}) + Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_link}) + + refute_receive {:DOWN, ^monitor_ref, :process, _pid, _reason} + end + end end From f4e2a36dce85c08354b8d9ab020dfe00d13c3fe0 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 14:41:36 +0100 Subject: [PATCH 17/33] Add handle_crash_group_down callback in Membrane.Bin --- lib/membrane/bin.ex | 16 ++++++++++++++++ lib/membrane/bin/callback_context.ex | 7 ++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/membrane/bin.ex b/lib/membrane/bin.ex index 21f439347..96d3579bd 100644 --- a/lib/membrane/bin.ex +++ b/lib/membrane/bin.ex @@ -173,6 +173,17 @@ defmodule Membrane.Bin do state :: state ) :: callback_return + @doc """ + Callback invoked when crash of the crash group happens. + + Context passed to this callback contains 2 additional fields: `:members` and `:crash_initiator`. + """ + @callback handle_crash_group_down( + group_name :: Child.group(), + context :: CallbackContext.t(), + state + ) :: callback_return + @doc """ A callback invoked when the bin is being removed by its parent. @@ -195,6 +206,7 @@ defmodule Membrane.Bin do handle_child_notification: 4, handle_parent_notification: 3, handle_tick: 3, + handle_crash_group_down: 3, handle_terminate_request: 2, handle_child_pad_removed: 4 @@ -336,6 +348,9 @@ defmodule Membrane.Bin do @impl true def handle_parent_notification(_notification, _ctx, state), do: {[], state} + @impl true + def handle_crash_group_down(_group_name, _ctx, state), do: {[], state} + @impl true def handle_terminate_request(_ctx, state), do: {[terminate: :normal], state} @@ -350,6 +365,7 @@ defmodule Membrane.Bin do handle_element_end_of_stream: 4, handle_child_notification: 4, handle_parent_notification: 3, + handle_crash_group_down: 3, handle_terminate_request: 2 end end diff --git a/lib/membrane/bin/callback_context.ex b/lib/membrane/bin/callback_context.ex index 9bcfd819c..e5c61ecf0 100644 --- a/lib/membrane/bin/callback_context.ex +++ b/lib/membrane/bin/callback_context.ex @@ -8,6 +8,9 @@ defmodule Membrane.Bin.CallbackContext do Field `:pad_options` is present only in `c:Membrane.Bin.handle_pad_added/3` and `c:Membrane.Bin.handle_pad_removed/3`. + + Fields `:members` and `:crash_initiator` are present only in + `c:Membrane.Pipeline.handle_crash_group_down/3`. """ @type t :: %{ :clock => Membrane.Clock.t(), @@ -18,6 +21,8 @@ defmodule Membrane.Bin.CallbackContext do :playback => Membrane.Playback.t(), :resource_guard => Membrane.ResourceGuard.t(), :utility_supervisor => Membrane.UtilitySupervisor.t(), - optional(:pad_options) => map() + optional(:pad_options) => map(), + optional(:members) => [Membrane.Child.name()], + optional(:crash_initiator) => Membrane.Child.name() } end From 36a10ddb6d136e6e5e0690822105298c0447d42a Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 14:54:53 +0100 Subject: [PATCH 18/33] Fix failing Testing.Pipeline tests --- test/membrane/testing/pipeline_test.exs | 40 +++++++++++++++++++------ 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/test/membrane/testing/pipeline_test.exs b/test/membrane/testing/pipeline_test.exs index 1939a4920..a61770fc6 100644 --- a/test/membrane/testing/pipeline_test.exs +++ b/test/membrane/testing/pipeline_test.exs @@ -22,10 +22,21 @@ defmodule Membrane.Testing.PipelineTest do test "works with :default implementation" do elements = [elem: Elem, elem2: Elem] links = [get_child(:elem) |> get_child(:elem2)] - options = [module: :default, spec: elements ++ links, test_process: nil] + + options = [ + module: :default, + spec: elements ++ links, + test_process: nil, + raise_on_child_pad_removed?: false + ] + assert {[spec: spec], state} = Pipeline.handle_init(%{}, options) - assert state == %Pipeline.State{module: nil, test_process: nil} + assert state == %Pipeline.State{ + module: nil, + test_process: nil, + raise_on_child_pad_removed?: false + } assert spec == elements ++ links end @@ -34,7 +45,12 @@ defmodule Membrane.Testing.PipelineTest do links = [child(:elem, Elem) |> child(:elem2, Elem)] options = [module: :default, spec: links, test_process: nil] assert {[spec: spec], state} = Pipeline.handle_init(%{}, options) - assert state == %Pipeline.State{module: nil, test_process: nil} + + assert state == %Pipeline.State{ + module: nil, + test_process: nil, + raise_on_child_pad_removed?: true + } assert spec == links end @@ -47,7 +63,8 @@ defmodule Membrane.Testing.PipelineTest do assert state == %Pipeline.State{ custom_pipeline_state: :state, module: MockPipeline, - test_process: nil + test_process: nil, + raise_on_child_pad_removed?: nil } end end @@ -57,7 +74,12 @@ defmodule Membrane.Testing.PipelineTest do links = [child(:elem, Elem) |> child(:elem2, Elem)] options = [module: :default, spec: links, test_process: nil] assert {[spec: spec], state} = Pipeline.handle_init(%{}, options) - assert state == %Pipeline.State{module: nil, test_process: nil} + + assert state == %Pipeline.State{ + module: nil, + test_process: nil, + raise_on_child_pad_removed?: true + } assert spec == links end @@ -85,7 +107,7 @@ defmodule Membrane.Testing.PipelineTest do end end - defmodule Bin do + defmodule TripleElementBin do use Membrane.Bin @impl true @@ -116,9 +138,9 @@ defmodule Membrane.Testing.PipelineTest do end spec = [ - child(:bin_1, Bin), - child(:bin_2, Bin), - child(:bin_3, Bin) + child(:bin_1, TripleElementBin), + child(:bin_2, TripleElementBin), + child(:bin_3, TripleElementBin) ] pipeline = Pipeline.start_supervised!(spec: spec) From 562459930598e60b47818a251e4e285a36f4042d Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 15:45:05 +0100 Subject: [PATCH 19/33] Fix typo --- lib/membrane/testing/pipeline.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/membrane/testing/pipeline.ex b/lib/membrane/testing/pipeline.ex index 172346a6c..e7da29f0e 100644 --- a/lib/membrane/testing/pipeline.ex +++ b/lib/membrane/testing/pipeline.ex @@ -480,7 +480,7 @@ defmodule Membrane.Testing.Pipeline do if state.raise_on_child_pad_removed? do raise """ Child #{inspect(child)} removed it's pad #{inspect(pad)}. If you want to - handle such a scenario, pass `raise_on_child_pad_removed?: true` option to + handle such a scenario, pass `raise_on_child_pad_removed?: false` option to `Membrane.Testing.Pipeline.start_*/2` or pass there a pipeline module implementing this callback via `:name` option. """ From ca06cd0cc44c12a97e9cfa477f78bbbd5aac670c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 17:21:42 +0100 Subject: [PATCH 20/33] Fix bug occuring when bin child dies between link_request and handle_link --- lib/membrane/core/bin/pad_controller.ex | 99 ++++++++-------- lib/membrane/core/element/pad_controller.ex | 4 +- .../child_life_controller/link_utils.ex | 17 ++- .../membrane/integration/child_crash_test.exs | 110 ++++++++++++++++++ 4 files changed, 172 insertions(+), 58 deletions(-) diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index 5fe2cc474..8735acd08 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -209,58 +209,61 @@ defmodule Membrane.Core.Bin.PadController do Core.Bin.State.t() ) :: {Core.Element.PadController.link_call_reply(), Core.Bin.State.t()} def handle_link(direction, endpoint, other_endpoint, params, state) do - pad_data = PadModel.get_data!(state, endpoint.pad_ref) - Membrane.Logger.debug("Handle link #{inspect(endpoint, pretty: true)}") - %{spec_ref: spec_ref, endpoint: child_endpoint, name: pad_name} = pad_data - - pad_props = - Map.merge(endpoint.pad_props, child_endpoint.pad_props, fn key, - external_value, - internal_value -> - if key in [ - :target_queue_size, - :min_demand_factor, - :auto_demand_size, - :toilet_capacity, - :throttling_factor - ] do - external_value || internal_value - else - internal_value - end - end) - - child_endpoint = %{child_endpoint | pad_props: pad_props} - - if params.initiator == :sibling do - :ok = - Child.PadController.validate_pad_mode!( - {endpoint.pad_ref, pad_data}, - {other_endpoint.pad_ref, params.other_info} - ) - end + with {:ok, pad_data} <- PadModel.get_data(state, endpoint.pad_ref) do + %{spec_ref: spec_ref, endpoint: child_endpoint, name: pad_name} = pad_data + + pad_props = + Map.merge(endpoint.pad_props, child_endpoint.pad_props, fn key, + external_value, + internal_value -> + if key in [ + :target_queue_size, + :min_demand_factor, + :auto_demand_size, + :toilet_capacity, + :throttling_factor + ] do + external_value || internal_value + else + internal_value + end + end) - params = - Map.update!( - params, - :stream_format_validation_params, - &[{state.module, pad_name} | &1] - ) + child_endpoint = %{child_endpoint | pad_props: pad_props} + + if params.initiator == :sibling do + :ok = + Child.PadController.validate_pad_mode!( + {endpoint.pad_ref, pad_data}, + {other_endpoint.pad_ref, params.other_info} + ) + end + + params = + Map.update!( + params, + :stream_format_validation_params, + &[{state.module, pad_name} | &1] + ) - reply = - Message.call!(child_endpoint.pid, :handle_link, [ - direction, - child_endpoint, - other_endpoint, - params - ]) - - state = PadModel.set_data!(state, endpoint.pad_ref, :linked?, true) - state = PadModel.set_data!(state, endpoint.pad_ref, :endpoint, child_endpoint) - state = ChildLifeController.proceed_spec_startup(spec_ref, state) - {reply, state} + reply = + Message.call!(child_endpoint.pid, :handle_link, [ + direction, + child_endpoint, + other_endpoint, + params + ]) + + state = PadModel.set_data!(state, endpoint.pad_ref, :linked?, true) + state = PadModel.set_data!(state, endpoint.pad_ref, :endpoint, child_endpoint) + state = ChildLifeController.proceed_spec_startup(spec_ref, state) + {reply, state} + else + {:error, :unknown_pad} -> + {{:error, {:unknown_pad, state.name, state.module, endpoint.pad_ref}}, state} + end end @doc """ diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex index a1ac7e87e..9f7f30eab 100644 --- a/lib/membrane/core/element/pad_controller.ex +++ b/lib/membrane/core/element/pad_controller.ex @@ -44,7 +44,9 @@ defmodule Membrane.Core.Element.PadController do {Endpoint.t(), PadModel.pad_info(), %{toilet: Toilet.t() | nil}} @type link_call_reply :: - :ok | {:ok, link_call_reply_props} | {:error, {:neighbor_dead, reason :: any}} + :ok + | {:ok, link_call_reply_props} + | {:error, {:neighbor_dead, reason :: any} | :unknown_pad} @default_auto_demand_size_factor 4000 diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 1a6a2e14f..a4e63f222 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -297,23 +297,22 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do if {Membrane.Bin, :itself} in [from.child, to.child] do state else - from_availability = Pad.availability_mode(from.pad_info.availability) - to_availability = Pad.availability_mode(to.pad_info.availability) params = %{initiator: :parent, stream_format_validation_params: []} case Message.call(from.pid, :handle_link, [:output, from, to, params]) do :ok -> put_in(state, [:links, link.id, :linked?], true) - {:error, {:call_failure, _reason}} when to_availability == :static -> - Process.exit(to.pid, :kill) - state + {:error, {:unknown_pad, name, module, pad}} -> + Membrane.Logger.debug(""" + Failed to establish link between #{inspect(from.pad_ref)} and #{inspect(to.pad_ref)} + because pad #{inspect(pad)} of component named #{inspect(name)} (#{inspect(module)}) + is down. + """) - {:error, {:neighbor_dead, _reason}} when from_availability == :static -> - Process.exit(from.pid, :kill) state - {:error, {:call_failure, _reason}} when to_availability == :dynamic -> + {:error, {:call_failure, _reason}} -> Membrane.Logger.debug(""" Failed to establish link between #{inspect(from.pad_ref)} and #{inspect(to.pad_ref)} because #{inspect(from.child)} is down. @@ -321,7 +320,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do state - {:error, {:neighbor_dead, _reason}} when from_availability == :dynamic -> + {:error, {:neighbor_dead, _reason}} -> Membrane.Logger.debug(""" Failed to establish link between #{inspect(from.pad_ref)} and #{inspect(to.pad_ref)} because #{inspect(to.child)} is down. diff --git a/test/membrane/integration/child_crash_test.exs b/test/membrane/integration/child_crash_test.exs index f397d5213..c08bab0df 100644 --- a/test/membrane/integration/child_crash_test.exs +++ b/test/membrane/integration/child_crash_test.exs @@ -1,11 +1,16 @@ defmodule Membrane.Integration.ChildCrashTest do use ExUnit.Case, async: false + import Membrane.ChildrenSpec import Membrane.Testing.Assertions + alias Membrane.Testing.PipelineTest.DynamicElement alias Membrane.Support.ChildCrashTest alias Membrane.Testing + require Membrane.Pad, as: Pad + require Membrane.Child, as: Child + test "Element that is not member of any crash group crashed when pipeline is in playing state" do Process.flag(:trap_exit, true) @@ -199,6 +204,111 @@ defmodule Membrane.Integration.ChildCrashTest do assert_pipeline_crash_group_down(pipeline_pid, 2) end + describe "When crash group inside a bin crashes" do + defmodule DynamicElement do + use Membrane.Endpoint + + def_input_pad :input, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + def_output_pad :output, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + @impl true + def handle_playing(_ctx, _opts) do + {[notify_parent: :playing], %{}} + end + end + + defmodule Bin do + use Membrane.Bin + + alias Membrane.Integration.ChildCrashTest.DynamicElement + require Membrane.Pad, as: Pad + + def_input_pad :input, + accepted_format: _any, + availability: :on_request + + def_output_pad :output, + accepted_format: _any, + availability: :on_request + + def_options do_internal_link: [spec: boolean(), default: true] + + @impl true + def handle_playing(_ctx, _opts) do + {[notify_parent: :playing], %{}} + end + + @impl true + def handle_pad_added(Pad.ref(direction, _id) = pad, _ctx, state) do + spec = + if state.do_internal_link do + [ + {child(direction, DynamicElement), group: :group, crash_group_mode: :temporary}, + get_child(Child.ref(direction, group: :group)) |> bin_output(pad) + ] + else + [] + end + + {[spec: spec, notify_parent: :handle_pad_added], state} + end + end + + test "bin removes a pad" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: child(:bin, Bin) |> child(:element, DynamicElement), + raise_on_child_pad_removed?: false + ) + + assert_pipeline_notified(pipeline, :element, :playing) + + child_ref = Child.ref(:output, group: :group) + bin_child_pid = Testing.Pipeline.get_child_pid!(pipeline, [:bin, child_ref]) + Process.exit(bin_child_pid, :kill) + + assert_child_pad_removed(pipeline, :bin, Pad.ref(:output, _id)) + + Testing.Pipeline.terminate(pipeline) + end + + @tag :dupa + test "spec is updated" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:first_bin, %Bin{do_internal_link: false}) + |> child(:second_bin, Bin) + |> child(:element, DynamicElement), + raise_on_child_pad_removed?: false + ) + + assert_pipeline_notified(pipeline, :second_bin, :handle_pad_added) + refute_pipeline_notified(pipeline, :element, :playing) + + child_ref = Child.ref(:output, group: :group) + + Testing.Pipeline.get_child_pid!(pipeline, [:second_bin, child_ref]) + |> Process.exit(:kill) + + assert_child_pad_removed(pipeline, :second_bin, Pad.ref(:output, _id)) + + Testing.Pipeline.execute_actions(pipeline, remove_children: :first_bin) + + assert_pipeline_notified(pipeline, :element, :playing) + assert_pipeline_notified(pipeline, :second_bin, :playing) + + Testing.Pipeline.terminate(pipeline) + end + end + defp assert_pid_alive(pid) do refute_receive {:EXIT, ^pid, _} end From d0c7d15a71ecb5c5e16e39ec303d3221e95f70d6 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 31 Jan 2023 17:34:24 +0100 Subject: [PATCH 21/33] Write test for many crash groups in a single spec --- .../membrane/integration/child_crash_test.exs | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/test/membrane/integration/child_crash_test.exs b/test/membrane/integration/child_crash_test.exs index c08bab0df..0c6f8f27e 100644 --- a/test/membrane/integration/child_crash_test.exs +++ b/test/membrane/integration/child_crash_test.exs @@ -279,7 +279,6 @@ defmodule Membrane.Integration.ChildCrashTest do Testing.Pipeline.terminate(pipeline) end - @tag :dupa test "spec is updated" do pipeline = Testing.Pipeline.start_link_supervised!( @@ -307,6 +306,39 @@ defmodule Membrane.Integration.ChildCrashTest do Testing.Pipeline.terminate(pipeline) end + + test "another crash group from this same spec is still living" do + children_definitions = + child(:first_bin, %Bin{do_internal_link: false}) + |> child(:second_bin, Bin) + |> child(:element, DynamicElement) + + spec = [ + {children_definitions, group: :a, crash_group_mode: :temporary}, + {children_definitions, group: :b, crash_group_mode: :temporary} + ] + + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: spec, + raise_on_child_pad_removed?: false + ) + + assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :a), :handle_pad_added) + + Testing.Pipeline.get_child_pid!(pipeline, Child.ref(:second_bin, group: :a)) + |> Process.exit(:kill) + + assert_pipeline_crash_group_down(pipeline, :a) + refute_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) + + Testing.Pipeline.execute_actions(pipeline, remove_children: Child.ref(:first_bin, group: :b)) + + assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) + assert_pipeline_notified(pipeline, Child.ref(:element, group: :b), :playing) + + Testing.Pipeline.terminate(pipeline) + end end defp assert_pid_alive(pid) do From d7b99c41e912aac0651e7665f37f08c7182f7586 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 1 Feb 2023 11:00:50 +0100 Subject: [PATCH 22/33] Refactor due to mix credo --- test/membrane/integration/child_crash_test.exs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/membrane/integration/child_crash_test.exs b/test/membrane/integration/child_crash_test.exs index 0c6f8f27e..a9e824259 100644 --- a/test/membrane/integration/child_crash_test.exs +++ b/test/membrane/integration/child_crash_test.exs @@ -4,7 +4,6 @@ defmodule Membrane.Integration.ChildCrashTest do import Membrane.ChildrenSpec import Membrane.Testing.Assertions - alias Membrane.Testing.PipelineTest.DynamicElement alias Membrane.Support.ChildCrashTest alias Membrane.Testing From ed165bc8802d2b6c5797af51ef53ed8977ced2b3 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 1 Feb 2023 12:03:11 +0100 Subject: [PATCH 23/33] Add more crash groups tests --- .../membrane/integration/child_crash_test.exs | 199 +++++++++++------- 1 file changed, 125 insertions(+), 74 deletions(-) diff --git a/test/membrane/integration/child_crash_test.exs b/test/membrane/integration/child_crash_test.exs index a9e824259..7a63a03e8 100644 --- a/test/membrane/integration/child_crash_test.exs +++ b/test/membrane/integration/child_crash_test.exs @@ -203,63 +203,89 @@ defmodule Membrane.Integration.ChildCrashTest do assert_pipeline_crash_group_down(pipeline_pid, 2) end - describe "When crash group inside a bin crashes" do - defmodule DynamicElement do - use Membrane.Endpoint - - def_input_pad :input, - accepted_format: _any, - availability: :on_request, - flow_control: :push - - def_output_pad :output, - accepted_format: _any, - availability: :on_request, - flow_control: :push - - @impl true - def handle_playing(_ctx, _opts) do - {[notify_parent: :playing], %{}} - end + defmodule DynamicElement do + use Membrane.Endpoint + + def_input_pad :input, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + def_output_pad :output, + accepted_format: _any, + availability: :on_request, + flow_control: :push + + @impl true + def handle_playing(_ctx, _opts) do + {[notify_parent: :playing], %{}} end + end + + defmodule Bin do + use Membrane.Bin + + alias Membrane.Integration.ChildCrashTest.DynamicElement + require Membrane.Pad, as: Pad + + def_input_pad :input, + accepted_format: _any, + availability: :on_request + + def_output_pad :output, + accepted_format: _any, + availability: :on_request - defmodule Bin do - use Membrane.Bin - - alias Membrane.Integration.ChildCrashTest.DynamicElement - require Membrane.Pad, as: Pad - - def_input_pad :input, - accepted_format: _any, - availability: :on_request - - def_output_pad :output, - accepted_format: _any, - availability: :on_request - - def_options do_internal_link: [spec: boolean(), default: true] - - @impl true - def handle_playing(_ctx, _opts) do - {[notify_parent: :playing], %{}} - end - - @impl true - def handle_pad_added(Pad.ref(direction, _id) = pad, _ctx, state) do - spec = - if state.do_internal_link do - [ - {child(direction, DynamicElement), group: :group, crash_group_mode: :temporary}, - get_child(Child.ref(direction, group: :group)) |> bin_output(pad) - ] - else - [] - end - - {[spec: spec, notify_parent: :handle_pad_added], state} - end + def_options do_internal_link: [spec: boolean(), default: true] + + @impl true + def handle_playing(_ctx, _opts) do + {[notify_parent: :playing], %{}} + end + + @impl true + def handle_pad_added(Pad.ref(direction, _id) = pad, _ctx, state) do + spec = + if state.do_internal_link do + [ + {child(direction, DynamicElement), group: :group, crash_group_mode: :temporary}, + get_child(Child.ref(direction, group: :group)) |> bin_output(pad) + ] + else + [] + end + + {[spec: spec, notify_parent: :handle_pad_added], state} end + end + + defmodule OuterBin do + use Membrane.Bin + alias Membrane.Integration.ChildCrashTest.Bin + + def_output_pad :output, + accepted_format: _any, + availability: :on_request + + @impl true + def handle_pad_added(pad, _ctx, state) do + spec = child(:bin, Bin) |> bin_output(pad) + {[spec: spec], state} + end + + @impl true + def handle_child_notification(notification, :bin, _ctx, state) do + {[notify_parent: {:child_notification, notification}], state} + end + + @impl true + def handle_child_pad_removed(child, pad, _ctx, state) do + {[notify_parent: {:child_pad_removed, child, pad}], state} + end + end + + describe "When crash group inside a bin crashes" do test "bin removes a pad" do pipeline = Testing.Pipeline.start_link_supervised!( @@ -306,40 +332,65 @@ defmodule Membrane.Integration.ChildCrashTest do Testing.Pipeline.terminate(pipeline) end - test "another crash group from this same spec is still living" do - children_definitions = - child(:first_bin, %Bin{do_internal_link: false}) - |> child(:second_bin, Bin) - |> child(:element, DynamicElement) - - spec = [ - {children_definitions, group: :a, crash_group_mode: :temporary}, - {children_definitions, group: :b, crash_group_mode: :temporary} - ] - + test "bin's parent's parent is notified, if should be" do pipeline = Testing.Pipeline.start_link_supervised!( - spec: spec, + spec: + child(:bin, OuterBin) + |> child(:element, DynamicElement), raise_on_child_pad_removed?: false ) - assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :a), :handle_pad_added) - - Testing.Pipeline.get_child_pid!(pipeline, Child.ref(:second_bin, group: :a)) - |> Process.exit(:kill) + assert_pipeline_notified(pipeline, :element, :playing) - assert_pipeline_crash_group_down(pipeline, :a) - refute_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) + inner_element_pid = + Testing.Pipeline.get_child_pid!( + pipeline, + [:bin, :bin, Child.ref(:output, group: :group)] + ) - Testing.Pipeline.execute_actions(pipeline, remove_children: Child.ref(:first_bin, group: :b)) + Process.exit(inner_element_pid, :kill) - assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) - assert_pipeline_notified(pipeline, Child.ref(:element, group: :b), :playing) + assert_child_pad_removed(pipeline, :bin, Pad.ref(:output, _id)) + assert_pipeline_notified(pipeline, :bin, {:child_pad_removed, :bin, Pad.ref(:output, _id)}) Testing.Pipeline.terminate(pipeline) end end + test "When crash group crashes, another crash group from this same spec is still living" do + children_definitions = + child(:first_bin, %Bin{do_internal_link: false}) + |> child(:second_bin, Bin) + |> child(:element, DynamicElement) + + spec = [ + {children_definitions, group: :a, crash_group_mode: :temporary}, + {children_definitions, group: :b, crash_group_mode: :temporary} + ] + + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: spec, + raise_on_child_pad_removed?: false + ) + + assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :a), :handle_pad_added) + + Testing.Pipeline.get_child_pid!(pipeline, Child.ref(:second_bin, group: :a)) + |> Process.exit(:kill) + + assert_pipeline_crash_group_down(pipeline, :a) + refute_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) + + Testing.Pipeline.execute_actions(pipeline, remove_children: Child.ref(:first_bin, group: :b)) + + assert_pipeline_notified(pipeline, Child.ref(:second_bin, group: :b), :playing) + assert_pipeline_notified(pipeline, Child.ref(:element, group: :b), :playing) + + Testing.Pipeline.terminate(pipeline) + end + defp assert_pid_alive(pid) do refute_receive {:EXIT, ^pid, _} end From c79239304cc67fa6b986bfed183db5acc2c46837 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 1 Feb 2023 12:25:02 +0100 Subject: [PATCH 24/33] Update docs --- CHANGELOG.md | 1 + lib/membrane/children_spec.ex | 7 +++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a45ed54..baff31d7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ * Remove _t suffix from types [#509](https://github.com/membraneframework/membrane_core/pull/509) * Implement automatic demands in Membrane Sinks and Endpoints. [#512](https://github.com/membraneframework/membrane_core/pull/512) * Add `handle_child_pad_removed/4` callback in Bins and Pipelines. [#513](https://github.com/membraneframework/membrane_core/pull/513) + * Introduce support for crash groups in Bins. [#521](https://github.com/membraneframework/membrane_core/pull/521) ## 0.11.0 * Separate element_name and pad arguments in handle_element_{start, end}_of_stream signature [#219](https://github.com/membraneframework/membrane_core/issues/219) diff --git a/lib/membrane/children_spec.ex b/lib/membrane/children_spec.ex index 08826bb89..e91371a20 100644 --- a/lib/membrane/children_spec.ex +++ b/lib/membrane/children_spec.ex @@ -225,7 +225,7 @@ defmodule Membrane.ChildrenSpec do #### Limitations At this moment crash groups are only useful for elements with dynamic pads. - Crash groups work only in pipelines and are not supported in bins. + Crash groups work in pipelines and bins as well. ### Log metadata `:log_metadata` field can be used to set the `Membrane.Logger` metadata for all children in the given children specification. @@ -237,9 +237,8 @@ defmodule Membrane.ChildrenSpec do ``` {[ child(:a, A) |> child(:b, B), - {child(:c, C), crash_group: - {:second, :temporary}} - ], crash_group_mode: :temporary, group: :first, node: some_node} + {child(:c, C), group: :second, crash_group_mode: :temporary} + ], group: :first, crash_group_mode: :temporary, node: some_node} ``` Child `:c` will be spawned in the `:second` crash group, while children `:a` and `:b` will be spawned in the `:first` crash group. From 94b0cadfe00dfd24e9848be82951807ad2a9c056 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Feliks=20Pobiedzi=C5=84ski?= <38541925+FelonEkonom@users.noreply.github.com> Date: Thu, 2 Feb 2023 15:38:29 +0100 Subject: [PATCH 25/33] Update lib/membrane/testing/assertions.ex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ɓukasz Kita --- lib/membrane/testing/assertions.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/membrane/testing/assertions.ex b/lib/membrane/testing/assertions.ex index 936fc71a0..28f4f111b 100644 --- a/lib/membrane/testing/assertions.ex +++ b/lib/membrane/testing/assertions.ex @@ -451,7 +451,7 @@ defmodule Membrane.Testing.Assertions do @doc """ Asserts that `Membrane.Testing.Pipeline` child with name `child` removed or is going to - remove it's pad with ref `pad` withing the `timeout` period specified in miliseconds. + remove it's pad with ref `pad` within the `timeout` period specified in milliseconds. """ defmacro assert_child_pad_removed( pipeline, From 89deeff770276d3b46eee3390533483b592a0a94 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 2 Feb 2023 17:00:24 +0100 Subject: [PATCH 26/33] Implement suggestion from CR --- lib/membrane/core/bin/pad_controller.ex | 107 +++++++++--------- .../core/parent/child_life_controller.ex | 77 +++++++------ .../child_life_controller/link_utils.ex | 38 +++---- lib/membrane/pad.ex | 6 + 4 files changed, 122 insertions(+), 106 deletions(-) diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index 5fe2cc474..c173af291 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -82,15 +82,15 @@ defmodule Membrane.Core.Bin.PadController do @spec remove_dynamic_pad!(Pad.ref(), State.t()) :: State.t() def remove_dynamic_pad!(pad_ref, state) do - case pad_ref do - Pad.ref(_name, _id) -> + cond do + Pad.is_dynamic_pad_ref(pad_ref) -> Message.send(state.parent_pid, :child_pad_removed, [state.name, pad_ref]) PadModel.delete_data!(state, pad_ref) - name when is_atom(name) and state.terminating? -> + Pad.is_static_pad_ref(pad_ref) and state.terminating? -> state - name when is_atom(name) -> + Pad.is_static_pad_ref(pad_ref) -> raise Membrane.PadError, "Tried to unlink bin static pad #{inspect(pad_ref)}. Static pads cannot be unlinked unless bin is terminating" end @@ -209,58 +209,61 @@ defmodule Membrane.Core.Bin.PadController do Core.Bin.State.t() ) :: {Core.Element.PadController.link_call_reply(), Core.Bin.State.t()} def handle_link(direction, endpoint, other_endpoint, params, state) do - pad_data = PadModel.get_data!(state, endpoint.pad_ref) - Membrane.Logger.debug("Handle link #{inspect(endpoint, pretty: true)}") - %{spec_ref: spec_ref, endpoint: child_endpoint, name: pad_name} = pad_data - - pad_props = - Map.merge(endpoint.pad_props, child_endpoint.pad_props, fn key, - external_value, - internal_value -> - if key in [ - :target_queue_size, - :min_demand_factor, - :auto_demand_size, - :toilet_capacity, - :throttling_factor - ] do - external_value || internal_value - else - internal_value - end - end) - - child_endpoint = %{child_endpoint | pad_props: pad_props} - - if params.initiator == :sibling do - :ok = - Child.PadController.validate_pad_mode!( - {endpoint.pad_ref, pad_data}, - {other_endpoint.pad_ref, params.other_info} - ) - end + with {:ok, pad_data} <- PadModel.get_data(state, endpoint.pad_ref) do + %{spec_ref: spec_ref, endpoint: child_endpoint, name: pad_name} = pad_data + + pad_props = + Map.merge(endpoint.pad_props, child_endpoint.pad_props, fn key, + external_value, + internal_value -> + if key in [ + :target_queue_size, + :min_demand_factor, + :auto_demand_size, + :toilet_capacity, + :throttling_factor + ] do + external_value || internal_value + else + internal_value + end + end) - params = - Map.update!( - params, - :stream_format_validation_params, - &[{state.module, pad_name} | &1] - ) + child_endpoint = %{child_endpoint | pad_props: pad_props} + + if params.initiator == :sibling do + :ok = + Child.PadController.validate_pad_mode!( + {endpoint.pad_ref, pad_data}, + {other_endpoint.pad_ref, params.other_info} + ) + end + + params = + Map.update!( + params, + :stream_format_validation_params, + &[{state.module, pad_name} | &1] + ) - reply = - Message.call!(child_endpoint.pid, :handle_link, [ - direction, - child_endpoint, - other_endpoint, - params - ]) - - state = PadModel.set_data!(state, endpoint.pad_ref, :linked?, true) - state = PadModel.set_data!(state, endpoint.pad_ref, :endpoint, child_endpoint) - state = ChildLifeController.proceed_spec_startup(spec_ref, state) - {reply, state} + reply = + Message.call!(child_endpoint.pid, :handle_link, [ + direction, + child_endpoint, + other_endpoint, + params + ]) + + state = PadModel.set_data!(state, endpoint.pad_ref, :linked?, true) + state = PadModel.set_data!(state, endpoint.pad_ref, :endpoint, child_endpoint) + state = ChildLifeController.proceed_spec_startup(spec_ref, state) + {reply, state} + else + {:error, :unknown_pad} -> + {{:error, {:unknown_pad, state.name, state.module, endpoint.pad_ref}}, state} + end end @doc """ diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index e2c32c6db..c4effdc3e 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -34,7 +34,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do children_names: [Child.name()], links_ids: [Link.id()], awaiting_responses: MapSet.t({Link.id(), Membrane.Pad.direction()}), - dependent_specs: %{spec_ref() => [Child.name()]} + dependencies: %{spec_ref() => [Child.name()]} } @type pending_specs :: %{spec_ref() => pending_spec()} @@ -97,7 +97,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do until all bin pads of the spec are linked. Linking bin pads is actually routing link calls to proper bin children. - Mark spec children as ready, optionally request to play or terminate - - Cleanup spec: remove it from `pending_specs` and all other specs' `dependent_specs` and try proceeding startup + - Cleanup spec: remove it from `pending_specs` and all other specs' `dependencies` and try proceeding startup for all other pending specs that depended on the spec. """ @spec handle_spec(ChildrenSpec.t(), Parent.state()) :: Parent.state() | no_return() @@ -144,7 +144,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do resolved_links = LinkUtils.resolve_links(links, spec_ref, state) state = %{state | links: Map.merge(state.links, Map.new(resolved_links, &{&1.id, &1}))} - dependent_specs = + dependencies = resolved_links |> Enum.flat_map(&[&1.from, &1.to]) |> Enum.map(&{&1.child_spec_ref, &1.child}) @@ -158,7 +158,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do status: :initializing, children_names: all_children_names, links_ids: Enum.map(links, & &1.id), - dependent_specs: dependent_specs, + dependencies: dependencies, awaiting_responses: MapSet.new() }) @@ -347,13 +347,13 @@ defmodule Membrane.Core.Parent.ChildLifeController do defp do_proceed_spec_startup(spec_ref, %{status: :initializing} = spec_data, state) do Membrane.Logger.debug( - "Proceeding spec #{inspect(spec_ref)} startup: initializing, dependent specs: #{inspect(Map.keys(spec_data.dependent_specs))}" + "Proceeding spec #{inspect(spec_ref)} startup: initializing, dependent specs: #{inspect(Map.keys(spec_data.dependencies))}" ) %{children: children} = state if Enum.all?(spec_data.children_names, &Map.fetch!(children, &1).initialized?) and - Enum.empty?(spec_data.dependent_specs) do + Enum.empty?(spec_data.dependencies) do Membrane.Logger.debug("Spec #{inspect(spec_ref)} status changed to initialized") do_proceed_spec_startup(spec_ref, %{spec_data | status: :initialized}, state) else @@ -566,20 +566,21 @@ defmodule Membrane.Core.Parent.ChildLifeController do |> Enum.reject(fn {link_id, _direction} -> link_id in children_links_ids_set end) |> MapSet.new() - dependent_specs = - spec_data.dependent_specs - |> Enum.map(fn {ref, spec_children} -> - {ref, Enum.reject(spec_children, &(&1 in children_set))} - end) - |> Enum.reject(&match?({_ref, []}, &1)) - |> Map.new() + dependencies = + spec_data.dependencies + |> update_spec_dependencies(children_set) + # |> Enum.map(fn {ref, spec_children} -> + # {ref, Enum.reject(spec_children, &(&1 in children_set))} + # end) + # |> Enum.reject(&match?({_ref, []}, &1)) + # |> Map.new() spec_data = %{ spec_data | children_names: children_names, links_ids: links_ids, awaiting_responses: awaiting_responses, - dependent_specs: dependent_specs + dependencies: dependencies } {spec_ref, spec_data} @@ -606,20 +607,22 @@ defmodule Membrane.Core.Parent.ChildLifeController do [link.from.child, link.to.child] end) - dependent_specs = + dependencies = [removed_link.from.child, removed_link.to.child] |> Enum.filter(&(&1 not in spec_links_endpoints)) |> case do [] -> - spec_data.dependent_specs + spec_data.dependencies endpoints_to_remove -> - spec_data.dependent_specs - |> Enum.map(fn {spec_ref, spec_children} -> - {spec_ref, Enum.reject(spec_children, &(&1 in endpoints_to_remove))} - end) - |> Enum.reject(&match?({_ref, []}, &1)) - |> Map.new() + spec_data.dependencies + |> update_spec_dependencies(endpoints_to_remove) + # spec_data.dependencies + # |> Enum.map(fn {spec_ref, spec_children} -> + # {spec_ref, Enum.reject(spec_children, &(&1 in endpoints_to_remove))} + # end) + # |> Enum.reject(&match?({_ref, []}, &1)) + # |> Map.new() end awaiting_responses = @@ -628,7 +631,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do spec_data = %{ spec_data - | dependent_specs: dependent_specs, + | dependencies: dependencies, links_ids: links_ids, awaiting_responses: awaiting_responses } @@ -640,6 +643,18 @@ defmodule Membrane.Core.Parent.ChildLifeController do end end + defp update_spec_dependencies(spec_dependencies, children_removed_from_spec) do + spec_dependencies + |> Enum.map(fn {spec_ref, spec_children} -> + { + spec_ref, + Enum.reject(spec_children, &(&1 in children_removed_from_spec)) + } + end) + |> Enum.reject(&match?({_ref, []}, &1)) + |> Map.new() + end + @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do # TODO: when spec is not ready yet, delete specific link from it and trigger proceeding @@ -691,12 +706,10 @@ defmodule Membrane.Core.Parent.ChildLifeController do %{pid: child_pid} = ChildrenModel.get_child_data!(state, child_name) with {:ok, group} <- CrashGroupUtils.get_group_by_member_pid(child_pid, state) do - existing_members = + state = group.members |> Enum.filter(&Map.has_key?(state.children, &1)) - - state = remove_children_from_specs(existing_members, state) - state = Enum.reduce(existing_members, state, &LinkUtils.unlink_element/2) + |> remove_children_from_specs(state) {result, state} = crash_all_group_members(group, child_name, state) @@ -746,15 +759,15 @@ defmodule Membrane.Core.Parent.ChildLifeController do end defp remove_spec_from_dependencies(spec_ref, state) do - dependent_specs = + dependencies = state.pending_specs - |> Enum.filter(fn {_ref, data} -> Map.has_key?(data.dependent_specs, spec_ref) end) + |> Enum.filter(fn {_ref, data} -> Map.has_key?(data.dependencies, spec_ref) end) |> Map.new(fn {ref, data} -> - {ref, Map.update!(data, :dependent_specs, &Map.delete(&1, spec_ref))} + {ref, Map.update!(data, :dependencies, &Map.delete(&1, spec_ref))} end) - state = %{state | pending_specs: Map.merge(state.pending_specs, dependent_specs)} - dependent_specs |> Map.keys() |> Enum.reduce(state, &proceed_spec_startup/2) + state = %{state | pending_specs: Map.merge(state.pending_specs, dependencies)} + dependencies |> Map.keys() |> Enum.reduce(state, &proceed_spec_startup/2) end defp exec_handle_crash_group_down_callback( diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 1a6a2e14f..0cd5cda5f 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -30,7 +30,9 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do %CrashGroup{members: members_names} = crash_group Enum.reduce(members_names, state, fn member_name, state -> - unlink_element(member_name, state) + with %{children: %{^member_name => _data}} <- state do + unlink_element(member_name, state) + end end) end @@ -58,19 +60,8 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do def remove_link(child_name, pad_ref, state) do with {:ok, link} <- get_link(state.links, child_name, pad_ref) do state = - if {Membrane.Bin, :itself} in [link.from.child, link.to.child] do - child_endpoint = opposite_endpoint(link, {Membrane.Bin, :itself}) - Message.send(child_endpoint.pid, :handle_unlink, child_endpoint.pad_ref) - - bin_endpoint = opposite_endpoint(link, child_endpoint.child) - PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) - else - for endpoint <- [link.from, link.to] do - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) - end - - state - end + [link.to, link.from] + |> Enum.reduce(state, &unlink_endpoint/2) state = ChildLifeController.remove_link_from_spec(link.id, state) delete_link(link, state) @@ -98,17 +89,20 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do state = %{state | links: Map.new(links, &{&1.id, &1})} Enum.reduce(dropped_links, state, fn link, state -> - case endpoint_to_unlink(child_name, link) do - %Endpoint{child: {Membrane.Bin, :itself}, pad_ref: pad_ref} -> - PadController.remove_dynamic_pad!(pad_ref, state) - - %Endpoint{} = endpoint -> - Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) - state - end + endpoint_to_unlink(child_name, link) + |> unlink_endpoint(state) end) end + defp unlink_endpoint(%Endpoint{child: {Membrane.Bin, :itself}} = endpoint, state) do + PadController.remove_dynamic_pad!(endpoint.pad_ref, state) + end + + defp unlink_endpoint(%Endpoint{} = endpoint, state) do + Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) + state + end + defp endpoint_to_unlink(child_name, %Link{from: %Endpoint{child: child_name}, to: to}), do: to defp endpoint_to_unlink(child_name, %Link{to: %Endpoint{child: child_name}, from: from}), diff --git a/lib/membrane/pad.ex b/lib/membrane/pad.ex index a6a927a23..6929a26c6 100644 --- a/lib/membrane/pad.ex +++ b/lib/membrane/pad.ex @@ -164,6 +164,12 @@ defmodule Membrane.Pad do (term |> is_tuple and term |> tuple_size == 3 and term |> elem(0) == __MODULE__ and term |> elem(1) |> is_atom) + + defguard is_static_pad_ref(term) when is_atom(term) + defguard is_dynamic_pad_ref(term) + when term |> is_tuple and term |> tuple_size == 3 and term |> elem(0) == __MODULE__ and + term |> elem(1) |> is_atom + defguard is_pad_name(term) when is_atom(term) defguard is_availability(term) when term in @availability_values From a6ee5f2d93666f9a4e621e41baa1629e68bca25e Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 2 Feb 2023 17:01:52 +0100 Subject: [PATCH 27/33] Remove done todo annotation --- lib/membrane/core/parent/child_life_controller.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index c4effdc3e..a5a50b256 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -657,7 +657,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do @spec handle_child_pad_removed(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() def handle_child_pad_removed(child, pad, state) do - # TODO: when spec is not ready yet, delete specific link from it and trigger proceeding Membrane.Logger.debug_verbose("Child #{inspect(child)} removed pad #{inspect(pad)}") Parent.ChildrenModel.assert_child_exists!(state, child) From df960e38bb8ba57c096d756a37ad7b660d8feb02 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 2 Feb 2023 17:02:31 +0100 Subject: [PATCH 28/33] Run format --- lib/membrane/core/parent/child_life_controller.ex | 12 +++++++----- lib/membrane/pad.ex | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index a5a50b256..6e00982b6 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -569,11 +569,12 @@ defmodule Membrane.Core.Parent.ChildLifeController do dependencies = spec_data.dependencies |> update_spec_dependencies(children_set) - # |> Enum.map(fn {ref, spec_children} -> - # {ref, Enum.reject(spec_children, &(&1 in children_set))} - # end) - # |> Enum.reject(&match?({_ref, []}, &1)) - # |> Map.new() + + # |> Enum.map(fn {ref, spec_children} -> + # {ref, Enum.reject(spec_children, &(&1 in children_set))} + # end) + # |> Enum.reject(&match?({_ref, []}, &1)) + # |> Map.new() spec_data = %{ spec_data @@ -617,6 +618,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do endpoints_to_remove -> spec_data.dependencies |> update_spec_dependencies(endpoints_to_remove) + # spec_data.dependencies # |> Enum.map(fn {spec_ref, spec_children} -> # {spec_ref, Enum.reject(spec_children, &(&1 in endpoints_to_remove))} diff --git a/lib/membrane/pad.ex b/lib/membrane/pad.ex index 6929a26c6..3b5b8bf1f 100644 --- a/lib/membrane/pad.ex +++ b/lib/membrane/pad.ex @@ -164,8 +164,8 @@ defmodule Membrane.Pad do (term |> is_tuple and term |> tuple_size == 3 and term |> elem(0) == __MODULE__ and term |> elem(1) |> is_atom) - defguard is_static_pad_ref(term) when is_atom(term) + defguard is_dynamic_pad_ref(term) when term |> is_tuple and term |> tuple_size == 3 and term |> elem(0) == __MODULE__ and term |> elem(1) |> is_atom From 703fa5a7c16fb6915d9ee0e961b547e822af813c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 2 Feb 2023 17:03:26 +0100 Subject: [PATCH 29/33] Remove commented lines of code --- lib/membrane/core/parent/child_life_controller.ex | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 6e00982b6..02ea80b6b 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -570,12 +570,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do spec_data.dependencies |> update_spec_dependencies(children_set) - # |> Enum.map(fn {ref, spec_children} -> - # {ref, Enum.reject(spec_children, &(&1 in children_set))} - # end) - # |> Enum.reject(&match?({_ref, []}, &1)) - # |> Map.new() - spec_data = %{ spec_data | children_names: children_names, @@ -618,13 +612,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do endpoints_to_remove -> spec_data.dependencies |> update_spec_dependencies(endpoints_to_remove) - - # spec_data.dependencies - # |> Enum.map(fn {spec_ref, spec_children} -> - # {spec_ref, Enum.reject(spec_children, &(&1 in endpoints_to_remove))} - # end) - # |> Enum.reject(&match?({_ref, []}, &1)) - # |> Map.new() end awaiting_responses = From 16a6832a3ee746829a07771110b45ab340b0b63c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 2 Feb 2023 17:42:03 +0100 Subject: [PATCH 30/33] Rename function remove_dynamic_pad! to remove_pad --- lib/membrane/core/bin/pad_controller.ex | 4 ++-- lib/membrane/core/parent/child_life_controller/link_utils.ex | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index c173af291..f2dc2c488 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -80,8 +80,8 @@ defmodule Membrane.Core.Bin.PadController do state end - @spec remove_dynamic_pad!(Pad.ref(), State.t()) :: State.t() - def remove_dynamic_pad!(pad_ref, state) do + @spec remove_pad!(Pad.ref(), State.t()) :: State.t() + def remove_pad!(pad_ref, state) do cond do Pad.is_dynamic_pad_ref(pad_ref) -> Message.send(state.parent_pid, :child_pad_removed, [state.name, pad_ref]) diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 0cd5cda5f..cc8d2baeb 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -44,7 +44,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do opposite_endpoint(link, child) |> case do %Endpoint{child: {Membrane.Bin, :itself}} = bin_endpoint -> - PadController.remove_dynamic_pad!(bin_endpoint.pad_ref, state) + PadController.remove_pad!(bin_endpoint.pad_ref, state) %Endpoint{} = endpoint -> Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) @@ -95,7 +95,7 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do end defp unlink_endpoint(%Endpoint{child: {Membrane.Bin, :itself}} = endpoint, state) do - PadController.remove_dynamic_pad!(endpoint.pad_ref, state) + PadController.remove_pad!(endpoint.pad_ref, state) end defp unlink_endpoint(%Endpoint{} = endpoint, state) do From f1eb3dc84fb190810b8f7ebf406302919983290b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 3 Feb 2023 15:00:15 +0100 Subject: [PATCH 31/33] Rename :remove_link action to :remove_child_pad --- CHANGELOG.md | 2 +- lib/membrane/bin.ex | 2 +- lib/membrane/bin/action.ex | 4 ++-- lib/membrane/core/bin/action_handler.ex | 2 +- lib/membrane/core/pipeline/action_handler.ex | 2 +- lib/membrane/pipeline/action.ex | 4 ++-- test/membrane/integration/child_pad_removed_test.exs | 6 +++--- test/membrane/integration/linking_test.exs | 6 +++--- test/membrane/testing/pipeline_test.exs | 8 ++++---- 9 files changed, 18 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baff31d7d..add321b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Changelog ## 1.0.0 - * Introduce `:remove_link` action in pipelines and bins. + * Introduce `:remove_child_pad` action in pipelines and bins. * Add children groups - a mechanism that allows refering to multiple children with a single identifier. * Rename `remove_child` action into `remove_children` and allow for removing a children group with a single action. * Add an ability to spawn anonymous children. diff --git a/lib/membrane/bin.ex b/lib/membrane/bin.ex index 96d3579bd..56812b3c8 100644 --- a/lib/membrane/bin.ex +++ b/lib/membrane/bin.ex @@ -93,7 +93,7 @@ defmodule Membrane.Bin do @doc """ Callback invoked when a child removes its pad. - Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()` + Removing child's pad due to return `t:Membrane.Bin.Action.remove_child_pad()` from `Membrane.Bin` callbacks does not trigger this callback. """ @callback handle_child_pad_removed( diff --git a/lib/membrane/bin/action.ex b/lib/membrane/bin/action.ex index 9f2388207..36ab7636a 100644 --- a/lib/membrane/bin/action.ex +++ b/lib/membrane/bin/action.ex @@ -58,7 +58,7 @@ defmodule Membrane.Bin.Action do Removed link has to have dynamic pads on both ends. """ - @type remove_link :: {:remove_link, {Child.name(), Pad.ref()}} + @type remove_child_pad :: {:remove_child_pad, {Child.name(), Pad.ref()}} @typedoc """ Starts a timer that will invoke `c:Membrane.Bin.handle_tick/3` callback @@ -134,7 +134,7 @@ defmodule Membrane.Bin.Action do | notify_parent | spec | remove_children - | remove_link + | remove_child_pad | start_timer | timer_interval | stop_timer diff --git a/lib/membrane/core/bin/action_handler.ex b/lib/membrane/core/bin/action_handler.ex index a4888c529..4134ae0e5 100644 --- a/lib/membrane/core/bin/action_handler.ex +++ b/lib/membrane/core/bin/action_handler.ex @@ -44,7 +44,7 @@ defmodule Membrane.Core.Bin.ActionHandler do end @impl CallbackHandler - def handle_action({:remove_link, {child_name, pad_ref}}, _cb, _params, state) do + def handle_action({:remove_child_pad, {child_name, pad_ref}}, _cb, _params, state) do Parent.ChildLifeController.handle_remove_link(child_name, pad_ref, state) end diff --git a/lib/membrane/core/pipeline/action_handler.ex b/lib/membrane/core/pipeline/action_handler.ex index b078dc021..d26d53bcc 100644 --- a/lib/membrane/core/pipeline/action_handler.ex +++ b/lib/membrane/core/pipeline/action_handler.ex @@ -49,7 +49,7 @@ defmodule Membrane.Core.Pipeline.ActionHandler do end @impl CallbackHandler - def handle_action({:remove_link, {child_name, pad_ref}}, _cb, _params, state) do + def handle_action({:remove_child_pad, {child_name, pad_ref}}, _cb, _params, state) do Parent.ChildLifeController.handle_remove_link(child_name, pad_ref, state) end diff --git a/lib/membrane/pipeline/action.ex b/lib/membrane/pipeline/action.ex index f41809f6e..c8c40f688 100644 --- a/lib/membrane/pipeline/action.ex +++ b/lib/membrane/pipeline/action.ex @@ -48,7 +48,7 @@ defmodule Membrane.Pipeline.Action do Removed link has to have dynamic pads on both ends. """ - @type remove_link :: {:remove_link, {Child.name(), Pad.ref()}} + @type remove_child_pad :: {:remove_child_pad, {Child.name(), Pad.ref()}} @typedoc """ Starts a timer that will invoke `c:Membrane.Pipeline.handle_tick/3` callback @@ -135,7 +135,7 @@ defmodule Membrane.Pipeline.Action do | notify_child | spec | remove_children - | remove_link + | remove_child_pad | start_timer | timer_interval | stop_timer diff --git a/test/membrane/integration/child_pad_removed_test.exs b/test/membrane/integration/child_pad_removed_test.exs index 86d8b00bd..1f7f97fd1 100644 --- a/test/membrane/integration/child_pad_removed_test.exs +++ b/test/membrane/integration/child_pad_removed_test.exs @@ -133,7 +133,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do test "sibling is unlinked" do for bin_actions <- [ [remove_children: :source], - [remove_link: {:source, Pad.ref(:output, 1)}] + [remove_child_pad: {:source, Pad.ref(:output, 1)}] ] do pipeline = start_link_pipeline!(DynamicBin, DynamicSink) @@ -158,7 +158,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do test "sibling linked via static pad raises" do for actions <- [ [remove_children: :source], - [remove_link: {:source, Pad.ref(:output, 1)}] + [remove_child_pad: {:source, Pad.ref(:output, 1)}] ] do pipeline = start_pipeline!(DynamicBin, StaticSink) @@ -181,7 +181,7 @@ defmodule Membrane.Integration.ChildPadRemovedTest do test "and sibling linked via static pad is removed, pipeline is not raising" do for bin_actions <- [ [remove_children: :source], - [remove_link: {:source, Pad.ref(:output, 1)}] + [remove_child_pad: {:source, Pad.ref(:output, 1)}] ] do pipeline = start_link_pipeline!(DynamicBin, StaticSink, remove_children: :sink) diff --git a/test/membrane/integration/linking_test.exs b/test/membrane/integration/linking_test.exs index 7985cfbed..d404d04a3 100644 --- a/test/membrane/integration/linking_test.exs +++ b/test/membrane/integration/linking_test.exs @@ -472,7 +472,7 @@ defmodule Membrane.Integration.LinkingTest do assert_start_of_stream(pipeline, :sink) end - test "Parent successfully unlinks children with dynamic pads using :remove_link action" do + test "Parent successfully unlinks children with dynamic pads using :remove_child_pad action" do spec = [ child(:source, __MODULE__.Element), @@ -490,8 +490,8 @@ defmodule Membrane.Integration.LinkingTest do for pad_id <- 1..10 do actions = if rem(pad_id, 2) == 0, - do: [remove_link: {:source, Pad.ref(:output, pad_id)}], - else: [remove_link: {:sink, Pad.ref(:input, pad_id)}] + do: [remove_child_pad: {:source, Pad.ref(:output, pad_id)}], + else: [remove_child_pad: {:sink, Pad.ref(:input, pad_id)}] Testing.Pipeline.execute_actions(pipeline, actions) diff --git a/test/membrane/testing/pipeline_test.exs b/test/membrane/testing/pipeline_test.exs index a61770fc6..42b10b16f 100644 --- a/test/membrane/testing/pipeline_test.exs +++ b/test/membrane/testing/pipeline_test.exs @@ -234,8 +234,8 @@ defmodule Membrane.Testing.PipelineTest do end @impl true - def handle_parent_notification(:remove_link, _ctx, state) do - {[remove_link: {:element, Pad.ref(:output, 1)}], state} + def handle_parent_notification(:remove_child_pad, _ctx, state) do + {[remove_child_pad: {:element, Pad.ref(:output, 1)}], state} end end @@ -248,7 +248,7 @@ defmodule Membrane.Testing.PipelineTest do monitor_ref = Process.monitor(pipeline) assert_pipeline_notified(pipeline, :sink, {:pad_added, _pad}) - Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_link}) + Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_child_pad}) assert_receive {:DOWN, ^monitor_ref, :process, _pid, _reason} end @@ -262,7 +262,7 @@ defmodule Membrane.Testing.PipelineTest do monitor_ref = Process.monitor(pipeline) assert_pipeline_notified(pipeline, :sink, {:pad_added, _pad}) - Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_link}) + Pipeline.execute_actions(pipeline, notify_child: {:bin, :remove_child_pad}) refute_receive {:DOWN, ^monitor_ref, :process, _pid, _reason} end From 152d42af16e39522aea1844adf7d62d8ed77049b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 3 Feb 2023 18:35:17 +0100 Subject: [PATCH 32/33] Cut link, if implcit_unlink?: false is passed in input pad options --- lib/membrane/children_spec.ex | 8 ++- lib/membrane/core/bin/action_handler.ex | 2 +- .../core/parent/child_life_controller.ex | 42 ++++++++++- .../child_life_controller/link_utils.ex | 71 +++++++++++-------- lib/membrane/core/parent/link.ex | 3 +- lib/membrane/core/pipeline/action_handler.ex | 4 +- lib/membrane/pipeline.ex | 2 +- 7 files changed, 92 insertions(+), 40 deletions(-) diff --git a/lib/membrane/children_spec.ex b/lib/membrane/children_spec.ex index e91371a20..4d5ec6a71 100644 --- a/lib/membrane/children_spec.ex +++ b/lib/membrane/children_spec.ex @@ -540,7 +540,8 @@ defmodule Membrane.ChildrenSpec do target_queue_size: number | nil, min_demand_factor: number | nil, auto_demand_size: number | nil, - throttling_factor: number | nil + throttling_factor: number | nil, + implicit_unlink?: boolean() ) :: builder() | no_return def via_in(builder, pad, props \\ []) @@ -565,7 +566,8 @@ defmodule Membrane.ChildrenSpec do min_demand_factor: [default: nil], auto_demand_size: [default: nil], toilet_capacity: [default: nil], - throttling_factor: [default: 1] + throttling_factor: [default: 1], + implicit_unlink?: [default: true] ) |> case do {:ok, props} -> @@ -578,7 +580,7 @@ defmodule Membrane.ChildrenSpec do if builder.status == :from_pad do builder else - via_out(builder, :output) + builder |> via_out(:output) end |> then(&%Builder{&1 | status: :to_pad, to_pad: pad, to_pad_props: Enum.into(props, %{})}) end diff --git a/lib/membrane/core/bin/action_handler.ex b/lib/membrane/core/bin/action_handler.ex index 4134ae0e5..1324ce3a5 100644 --- a/lib/membrane/core/bin/action_handler.ex +++ b/lib/membrane/core/bin/action_handler.ex @@ -45,7 +45,7 @@ defmodule Membrane.Core.Bin.ActionHandler do @impl CallbackHandler def handle_action({:remove_child_pad, {child_name, pad_ref}}, _cb, _params, state) do - Parent.ChildLifeController.handle_remove_link(child_name, pad_ref, state) + Parent.ChildLifeController.handle_remove_child_pad(child_name, pad_ref, state) end @impl CallbackHandler diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 02ea80b6b..e71078717 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -15,6 +15,8 @@ defmodule Membrane.Core.Parent.ChildLifeController do SpecificationParser } + alias Membrane.Core.Parent.Link.Endpoint + alias Membrane.Pad alias Membrane.ParentError @@ -530,10 +532,44 @@ defmodule Membrane.Core.Parent.ChildLifeController do Parent.ChildrenModel.update_children!(state, refs, &%{&1 | terminating?: true}) end - @spec handle_remove_link(Child.name(), Pad.ref(), Parent.state()) :: + @spec handle_remove_child_pad(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() - def handle_remove_link(child_name, pad_ref, state) do - LinkUtils.remove_link(child_name, pad_ref, state) + def handle_remove_child_pad(child, pad, state) do + Enum.find(state.links, fn {_id, link} -> + link_contanins_child_with_pad?(link, child, pad) + end) + |> case do + {_id, + %Link{ + from: %Endpoint{child: ^child, pad_ref: ^pad}, + to: %Endpoint{pad_props: %{implicit_unlink?: false}} + } = link} -> + # TODO: handle case when spec is not ready yet + LinkUtils.unlink_endpoint(link.from, state) + |> update_in([:links, link.id], &%{&1 | from: nil, cut?: true}) + + {_id, %Link{} = link} -> + LinkUtils.remove_link!(link.id, state) + + nil -> + if Map.has_key?(state.children, child) do + raise ParentError, """ + Attempted to unlink pad #{inspect(pad)} of child #{inspect(child)}, but this child does not have this pad linked + """ + end + + raise ParentError, """ + Attempted to unlink pad #{inspect(pad)} of child #{inspect(child)}, but such a child does not exist + """ + end + end + + defp link_contanins_child_with_pad?(link, child, pad) do + case link do + %Link{from: %Endpoint{child: ^child, pad_ref: ^pad}} -> true + %Link{to: %Endpoint{child: ^child, pad_ref: ^pad}} -> true + %Link{} -> false + end end defp remove_children_from_specs(children, state) do diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index e4dd2560e..3753fc10a 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -18,7 +18,6 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do alias Membrane.Core.Parent.Link.Endpoint alias Membrane.LinkError alias Membrane.Pad - alias Membrane.ParentError require Membrane.Core.Message require Membrane.Core.Telemetry @@ -56,49 +55,63 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do delete_link(link, state) end - @spec remove_link(Child.name(), Pad.ref(), Parent.state()) :: Parent.state() - def remove_link(child_name, pad_ref, state) do - with {:ok, link} <- get_link(state.links, child_name, pad_ref) do - state = - [link.to, link.from] - |> Enum.reduce(state, &unlink_endpoint/2) + @spec remove_link!(Link.id(), Parent.state()) :: Parent.state() + def remove_link!(link_id, state) do + link = Map.fetch!(state.links, link_id) - state = ChildLifeController.remove_link_from_spec(link.id, state) - delete_link(link, state) - else - {:error, :not_found} -> - with %{^child_name => _child_entry} <- state.children do - raise ParentError, """ - Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but this child does not have this pad linked - """ - end + state = + [link.to, link.from] + |> Enum.reduce(state, &unlink_endpoint/2) - raise ParentError, """ - Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but such a child does not exist - """ - end + state = ChildLifeController.remove_link_from_spec(link_id, state) + delete_link(link, state) end @spec unlink_element(Child.name(), Parent.state()) :: Parent.state() def unlink_element(child_name, state) do - {dropped_links, links} = + grouped_links = state.links |> Map.values() - |> Enum.split_with(&(child_name in [&1.from.child, &1.to.child])) + |> Enum.group_by(fn + %Link{from: %{child: ^child_name}, to: %{pad_props: %{implicit_unlink?: false}}} -> + :cut_links - state = %{state | links: Map.new(links, &{&1.id, &1})} + %Link{from: %{child: ^child_name}} -> + :deleted_links - Enum.reduce(dropped_links, state, fn link, state -> - endpoint_to_unlink(child_name, link) - |> unlink_endpoint(state) - end) + %Link{to: %{child: ^child_name}} -> + :deleted_links + + %Link{} -> + :unchanged_links + end) + + deleted_links = Map.get(grouped_links, :deleted_links, []) + cut_links = Map.get(grouped_links, :cut_links, []) + unchanged_links = Map.get(grouped_links, :unchanged_links, []) + + state = + Enum.reduce(deleted_links, state, fn link, state -> + case endpoint_to_unlink(child_name, link) do + %Endpoint{} = endpoint -> unlink_endpoint(endpoint, state) + nil -> state + end + end) + + links = + Enum.map(cut_links, &%{&1 | from: nil, cut?: true}) + |> Enum.concat(unchanged_links) + |> Map.new(&{&1.id, &1}) + + %{state | links: links} end - defp unlink_endpoint(%Endpoint{child: {Membrane.Bin, :itself}} = endpoint, state) do + @spec unlink_endpoint(Endpoint.t(), Parent.state()) :: Parent.state() + def unlink_endpoint(%Endpoint{child: {Membrane.Bin, :itself}} = endpoint, state) do PadController.remove_pad!(endpoint.pad_ref, state) end - defp unlink_endpoint(%Endpoint{} = endpoint, state) do + def unlink_endpoint(%Endpoint{} = endpoint, state) do Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) state end diff --git a/lib/membrane/core/parent/link.ex b/lib/membrane/core/parent/link.ex index 2e078b5c4..27d5beab0 100644 --- a/lib/membrane/core/parent/link.ex +++ b/lib/membrane/core/parent/link.ex @@ -6,7 +6,7 @@ defmodule Membrane.Core.Parent.Link do alias __MODULE__.Endpoint @enforce_keys [:id, :from, :to] - defstruct @enforce_keys ++ [linked?: false, spec_ref: nil] + defstruct @enforce_keys ++ [linked?: false, spec_ref: nil, cut?: false] @type id :: reference() @@ -15,6 +15,7 @@ defmodule Membrane.Core.Parent.Link do from: Endpoint.t(), to: Endpoint.t(), linked?: boolean(), + cut?: boolean(), spec_ref: Membrane.Core.Parent.ChildLifeController.spec_ref() } end diff --git a/lib/membrane/core/pipeline/action_handler.ex b/lib/membrane/core/pipeline/action_handler.ex index d26d53bcc..d13e42cd0 100644 --- a/lib/membrane/core/pipeline/action_handler.ex +++ b/lib/membrane/core/pipeline/action_handler.ex @@ -49,8 +49,8 @@ defmodule Membrane.Core.Pipeline.ActionHandler do end @impl CallbackHandler - def handle_action({:remove_child_pad, {child_name, pad_ref}}, _cb, _params, state) do - Parent.ChildLifeController.handle_remove_link(child_name, pad_ref, state) + def handle_action({:remove_child_pad, {child, pad}}, _cb, _params, state) do + Parent.ChildLifeController.handle_remove_child_pad(child, pad, state) end @impl CallbackHandler diff --git a/lib/membrane/pipeline.ex b/lib/membrane/pipeline.ex index a81d00146..11100ca45 100644 --- a/lib/membrane/pipeline.ex +++ b/lib/membrane/pipeline.ex @@ -138,7 +138,7 @@ defmodule Membrane.Pipeline do @doc """ Callback invoked when a child removes its pad. - Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()` + Removing child's pad due to return `t:Membrane.Bin.Action.remove_child_pad()` from `Membrane.Pipeline` callbacks does not trigger this callback. """ @callback handle_child_pad_removed( From 5067470eaa5ae017948006414a714f2dfd32ac5b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 6 Feb 2023 17:51:49 +0100 Subject: [PATCH 33/33] wip --- lib/membrane/core/bin/pad_controller.ex | 7 +- .../child_life_controller/link_utils.ex | 5 +- .../integration/cutting_link_test.exs | 327 ++++++++++++++++++ 3 files changed, 334 insertions(+), 5 deletions(-) create mode 100644 test/membrane/integration/cutting_link_test.exs diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex index f2dc2c488..c8bb7eaef 100644 --- a/lib/membrane/core/bin/pad_controller.ex +++ b/lib/membrane/core/bin/pad_controller.ex @@ -83,13 +83,13 @@ defmodule Membrane.Core.Bin.PadController do @spec remove_pad!(Pad.ref(), State.t()) :: State.t() def remove_pad!(pad_ref, state) do cond do + state.terminating? -> + state + Pad.is_dynamic_pad_ref(pad_ref) -> Message.send(state.parent_pid, :child_pad_removed, [state.name, pad_ref]) PadModel.delete_data!(state, pad_ref) - Pad.is_static_pad_ref(pad_ref) and state.terminating? -> - state - Pad.is_static_pad_ref(pad_ref) -> raise Membrane.PadError, "Tried to unlink bin static pad #{inspect(pad_ref)}. Static pads cannot be unlinked unless bin is terminating" @@ -277,6 +277,7 @@ defmodule Membrane.Core.Bin.PadController do {pad_data, state} = PadModel.pop_data!(state, pad_ref) if endpoint do + IO.inspect(endpoint, label: "ENDPOINT") Message.send(endpoint.pid, :handle_unlink, endpoint.pad_ref) ChildLifeController.proceed_spec_startup(pad_data.spec_ref, state) else diff --git a/lib/membrane/core/parent/child_life_controller/link_utils.ex b/lib/membrane/core/parent/child_life_controller/link_utils.ex index 3753fc10a..c36daf9c1 100644 --- a/lib/membrane/core/parent/child_life_controller/link_utils.ex +++ b/lib/membrane/core/parent/child_life_controller/link_utils.ex @@ -60,7 +60,8 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do link = Map.fetch!(state.links, link_id) state = - [link.to, link.from] + [link.from, link.to] + |> Enum.reject(&(&1 == nil)) |> Enum.reduce(state, &unlink_endpoint/2) state = ChildLifeController.remove_link_from_spec(link_id, state) @@ -86,8 +87,8 @@ defmodule Membrane.Core.Parent.ChildLifeController.LinkUtils do :unchanged_links end) - deleted_links = Map.get(grouped_links, :deleted_links, []) cut_links = Map.get(grouped_links, :cut_links, []) + deleted_links = Map.get(grouped_links, :deleted_links, []) unchanged_links = Map.get(grouped_links, :unchanged_links, []) state = diff --git a/test/membrane/integration/cutting_link_test.exs b/test/membrane/integration/cutting_link_test.exs new file mode 100644 index 000000000..e7e2f206c --- /dev/null +++ b/test/membrane/integration/cutting_link_test.exs @@ -0,0 +1,327 @@ +defmodule Membrane.Integration.CuttingLinksTest do + use Bunch + use ExUnit.Case, async: false + + import Membrane.ChildrenSpec + import Membrane.Testing.Assertions + + alias Membrane.Testing + + require Membrane.Pad, as: Pad + + defmodule Macros do + defmacro notify_on_playing() do + quote do + @impl true + def handle_playing(_ctx, state) do + {[notify_parent: :playing], state} + end + end + end + + defmacro notify_on_pad_removed() do + quote do + @impl true + def handle_pad_removed(pad, _ctx, state) do + {[notify_parent: {:pad_removed, pad}], state} + end + end + end + + defmacro forward_child_notification() do + quote do + @impl true + def handle_child_notification(msg, child, _ctx, state) do + {[notify_parent: {:child_notification, child, msg}], state} + end + end + end + end + + defmodule DynamicElement do + use Membrane.Endpoint + require Membrane.Integration.CuttingLinksTest.Macros, as: Macros + + def_input_pad :input, accepted_format: _any, flow_control: :push, availability: :on_request + def_output_pad :output, accepted_format: _any, flow_control: :push, availability: :on_request + + Macros.notify_on_playing() + Macros.notify_on_pad_removed() + end + + defmodule StaticSource do + use Membrane.Source + def_output_pad :output, accepted_format: _any, flow_control: :push + end + + defmodule StaticSink do + use Membrane.Sink + def_input_pad :input, accepted_format: _any, flow_control: :push + end + + defmodule DynamicBin do + use Membrane.Bin + require Membrane.Integration.CuttingLinksTest.Macros, as: Macros + + def_input_pad :input, accepted_format: _any, availability: :on_request + def_output_pad :output, accepted_format: _any, availability: :on_request + + Macros.notify_on_playing() + Macros.notify_on_pad_removed() + Macros.forward_child_notification() + + @impl true + def handle_init(_ctx, state) do + {[spec: child(:element, DynamicElement)], state} + end + + @impl true + def handle_pad_added(Pad.ref(direction, _id) = pad, _ctx, state) do + spec = + case direction do + :input -> bin_input(pad) |> get_child(:element) + :output -> get_child(:element) |> bin_output(pad) + end + + {[spec: spec], state} + end + end + + defmodule NestedBin do + use Membrane.Bin + require Membrane.Integration.CuttingLinksTest.Macros, as: Macros + + def_input_pad :input, accepted_format: _any, availability: :on_request + + Macros.notify_on_playing() + Macros.notify_on_pad_removed() + Macros.forward_child_notification() + + @impl true + def handle_init(_ctx, state) do + {[spec: child(:inner_bin, DynamicBin)], state} + end + + @impl true + def handle_pad_added(Pad.ref(:input, _id) = pad, _ctx, state) do + spec = + bin_input(pad) + |> via_in(pad, implicit_unlink?: false) + |> get_child(:inner_bin) + + {[spec: spec], state} + end + end + + describe "cutting link between 2 elements" do + test "with dynamic pads" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:source, DynamicElement) + |> via_in(Pad.ref(:input, 1), implicit_unlink?: false) + |> child(:sink, DynamicElement) + ) + + assert_pipeline_notified(pipeline, :source, :playing) + assert_pipeline_notified(pipeline, :sink, :playing) + + Testing.Pipeline.execute_actions(pipeline, remove_children: :source) + refute_pipeline_notified(pipeline, :sink, {:pad_removed, _pad}) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:sink, Pad.ref(:input, 1)}) + assert_pipeline_notified(pipeline, :sink, {:pad_removed, Pad.ref(:input, 1)}) + + Testing.Pipeline.terminate(pipeline) + end + + test "when `source` side has a static pad" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:source, StaticSource) + |> via_in(Pad.ref(:input, 1), implicit_unlink?: false) + |> child(:sink, DynamicElement) + ) + + assert_pipeline_notified(pipeline, :sink, :playing) + + Testing.Pipeline.execute_actions(pipeline, remove_children: :source) + refute_pipeline_notified(pipeline, :sink, {:pad_removed, _pad}) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:sink, Pad.ref(:input, 1)}) + assert_pipeline_notified(pipeline, :sink, {:pad_removed, Pad.ref(:input, 1)}) + + Testing.Pipeline.terminate(pipeline) + end + + test "when `sink` side has a static pad" do + pipeline = + Testing.Pipeline.start_supervised!( + spec: + child(:source, DynamicElement) + |> via_out(Pad.ref(:output, 1)) + |> via_in(:input, implicit_unlink?: false) + |> child(:sink, StaticSink) + ) + + assert_pipeline_notified(pipeline, :source, :playing) + + sink_pid = Testing.Pipeline.get_child_pid!(pipeline, :sink) + sink_monitor = Process.monitor(sink_pid) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:source, Pad.ref(:output, 1)}) + refute_receive {:DOWN, ^sink_monitor, :process, ^sink_pid, _reason} + + pipeline_monitor = Process.monitor(pipeline) + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:sink, :input}) + + assert_receive {:DOWN, ^pipeline_monitor, :process, ^pipeline, {:shutdown, :child_crash}} + end + end + + test "cutting link between 2 bins with dynamic pads" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:source_bin, DynamicBin) + |> via_in(Pad.ref(:input, 1), implicit_unlink?: false) + |> child(:sink_bin, DynamicBin) + ) + + assert_pipeline_notified(pipeline, :source_bin, :playing) + assert_pipeline_notified(pipeline, :source_bin, {:child_notification, :element, :playing}) + assert_pipeline_notified(pipeline, :sink_bin, :playing) + assert_pipeline_notified(pipeline, :sink_bin, {:child_notification, :element, :playing}) + + Testing.Pipeline.execute_actions(pipeline, remove_children: :source_bin) + refute_pipeline_notified(pipeline, :sink_bin, {:pad_removed, _pad}) + + refute_pipeline_notified( + pipeline, + :sink_bin, + {:child_notification, :element, {:pad_removed, _pad}} + ) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:sink_bin, Pad.ref(:input, 1)}) + assert_pipeline_notified(pipeline, :sink_bin, {:pad_removed, Pad.ref(:input, 1)}) + + assert_pipeline_notified( + pipeline, + :sink_bin, + {:child_notification, :element, {:pad_removed, Pad.ref(:input, _id)}} + ) + + Testing.Pipeline.terminate(pipeline) + end + + describe "cutting link between bin's child and bin's pad" do + test "triggered by unlinking bin" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:element, DynamicElement) + |> via_out(Pad.ref(:output, 1)) + |> via_in(Pad.ref(:input, 1)) + |> child(:bin, NestedBin) + ) + + assert_pipeline_notified(pipeline, :element, :playing) + assert_pipeline_notified(pipeline, :bin, :playing) + assert_pipeline_notified(pipeline, :bin, {:child_notification, :inner_bin, :playing}) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:child_notification, :element, :playing}} + ) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:element, Pad.ref(:output, 1)}) + + assert_pipeline_notified(pipeline, :bin, {:pad_removed, Pad.ref(:input, 1)}) + + refute_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:pad_removed, _pad}} + ) + + refute_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:child_notification, :element, {:pad_removed, _pad}}} + ) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:bin, Pad.ref(:input, 1)}) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:pad_removed, Pad.ref(:input, _id)}} + ) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, + {:child_notification, :element, {:pad_removed, Pad.ref(:input, _id)}}} + ) + end + + @tag :skip + test "triggered by pipeline removing bin's pad" do + test "triggered by unlinking bin" do + pipeline = + Testing.Pipeline.start_link_supervised!( + spec: + child(:element, DynamicElement) + |> via_out(Pad.ref(:output, 1)) + |> via_in(Pad.ref(:input, 1)) + |> child(:bin, NestedBin) + ) + + assert_pipeline_notified(pipeline, :element, :playing) + assert_pipeline_notified(pipeline, :bin, :playing) + assert_pipeline_notified(pipeline, :bin, {:child_notification, :inner_bin, :playing}) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:child_notification, :element, :playing}} + ) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:bin, Pad.ref(:input, 1)}) + + assert_pipeline_notified(pipeline, :bin, {:pad_removed, Pad.ref(:input, 1)}) + + refute_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:pad_removed, _pad}} + ) + + refute_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:child_notification, :element, {:pad_removed, _pad}}} + ) + + Testing.Pipeline.execute_actions(pipeline, remove_child_pad: {:bin, Pad.ref(:input, 1)}) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, {:pad_removed, Pad.ref(:input, _id)}} + ) + + assert_pipeline_notified( + pipeline, + :bin, + {:child_notification, :inner_bin, + {:child_notification, :element, {:pad_removed, Pad.ref(:input, _id)}}} + ) + end + + end + end +end