From 47e97c925966405730df4487e128e503950010ad Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 2 Feb 2024 15:12:53 +0100 Subject: [PATCH] Stopt calling handle_spec_started in between handling actions --- lib/membrane/core/bin/action_handler.ex | 15 ++++++++++- lib/membrane/core/bin/state.ex | 2 ++ .../core/parent/child_life_controller.ex | 18 +++++++++++-- lib/membrane/core/pipeline/action_handler.ex | 25 +++++++++++++------ lib/membrane/core/pipeline/state.ex | 2 ++ test/membrane/core/pipeline_test.exs | 2 ++ 6 files changed, 53 insertions(+), 11 deletions(-) diff --git a/lib/membrane/core/bin/action_handler.ex b/lib/membrane/core/bin/action_handler.ex index a4888c529..bd507ca41 100644 --- a/lib/membrane/core/bin/action_handler.ex +++ b/lib/membrane/core/bin/action_handler.ex @@ -6,6 +6,7 @@ defmodule Membrane.Core.Bin.ActionHandler do alias Membrane.Core alias Membrane.Core.Bin.State alias Membrane.Core.{Message, Parent, TimerController} + alias Membrane.Core.Parent.ChildLifeController require Membrane.Logger require Message @@ -34,7 +35,14 @@ defmodule Membrane.Core.Bin.ActionHandler do end @impl CallbackHandler - def handle_action({:spec, spec}, _cb, _params, state) do + def handle_action({:spec, spec}, cb, _params, state) do + if cb == :handle_spec_started do + Membrane.Logger.warning(""" + Action :spec was returned from handle_spec_started/3 callback. It is suggested not to do this, + because it might lead to infinite loof of handle_spec_started/3 executions. + """) + end + Parent.ChildLifeController.handle_spec(spec, state) end @@ -108,4 +116,9 @@ defmodule Membrane.Core.Bin.ActionHandler do def handle_action(action, _callback, _params, _state) do raise ActionError, action: action, reason: {:unknown_action, Membrane.Bin.Action} end + + @impl CallbackHandler + def handle_end_of_actions(state) do + ChildLifeController.trigger_specs(state) + end end diff --git a/lib/membrane/core/bin/state.ex b/lib/membrane/core/bin/state.ex index 9ac285763..eacdd9b8d 100644 --- a/lib/membrane/core/bin/state.ex +++ b/lib/membrane/core/bin/state.ex @@ -40,6 +40,7 @@ defmodule Membrane.Core.Bin.State do }, children_log_metadata: Keyword.t(), pending_specs: ChildLifeController.pending_specs(), + specs_to_trigger: [ChildLifeController.spec_ref()], playback: Membrane.Playback.t(), initialized?: boolean(), terminating?: boolean(), @@ -69,6 +70,7 @@ defmodule Membrane.Core.Bin.State do links: %{}, crash_groups: %{}, pending_specs: %{}, + specs_to_trigger: [], synchronization: nil, initialized?: false, terminating?: false, diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 794c5ea64..6624e5644 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -71,6 +71,12 @@ defmodule Membrane.Core.Parent.ChildLifeController do log_metadata: [] } + # todo: handle documentation + @spec handle_spec(ChildrenSpec.t(), Parent.state()) :: Parent.state() + def handle_spec(spec, state) do + Map.update!(state, :specs_to_trigger, &[spec | &1]) + end + @doc """ Handles `Membrane.ChildrenSpec` returned with `spec` action. @@ -99,8 +105,16 @@ defmodule Membrane.Core.Parent.ChildLifeController do - Cleanup spec: remove it from `pending_specs` and all other specs' `dependent_specs` 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() - def handle_spec(spec, state) do + @spec trigger_specs(Parent.state()) :: Parent.state() | no_return() + def trigger_specs(state) do + specs_to_trigger = state.specs_to_trigger + state = %{state | specs_to_trigger: []} + + List.foldr(specs_to_trigger, state, &do_trigger_spec/2) + end + + @spec do_trigger_spec(ChildrenSpec.t(), Parent.state()) :: Parent.state() | no_return() + defp do_trigger_spec(spec, state) do spec_ref = make_ref() canonical_spec = make_canonical(spec) diff --git a/lib/membrane/core/pipeline/action_handler.ex b/lib/membrane/core/pipeline/action_handler.ex index 7fc2faca7..bc3ec14a0 100644 --- a/lib/membrane/core/pipeline/action_handler.ex +++ b/lib/membrane/core/pipeline/action_handler.ex @@ -5,9 +5,11 @@ defmodule Membrane.Core.Pipeline.ActionHandler do alias Membrane.ActionError alias Membrane.Core alias Membrane.Core.{Parent, TimerController} - alias Membrane.Core.Parent.LifecycleController + alias Membrane.Core.Parent.{ChildLifeController, LifecycleController} alias Membrane.Core.Pipeline.State + require Membrane.Logger + @impl CallbackHandler def handle_action({:spec, args}, _cb, _params, %State{terminating?: true}) do raise Membrane.ParentError, @@ -32,7 +34,14 @@ defmodule Membrane.Core.Pipeline.ActionHandler do end @impl CallbackHandler - def handle_action({:spec, spec}, _cb, _params, state) do + def handle_action({:spec, spec}, cb, _params, state) do + if cb == :handle_spec_started do + Membrane.Logger.warning(""" + Action :spec was returned from handle_spec_started/3 callback. It is suggested not to do this, + because it might lead to infinite loof of handle_spec_started/3 executions. + """) + end + Parent.ChildLifeController.handle_spec(spec, state) end @@ -105,11 +114,11 @@ defmodule Membrane.Core.Pipeline.ActionHandler do end @impl CallbackHandler - def handle_end_of_actions(state) when state.awaiting_setup_completition? do - %{state | awaiting_setup_completition?: false} - |> Membrane.Core.LifecycleController.complete_setup() + def handle_end_of_actions(state) do + with %{awaiting_setup_completition?: true} <- state do + %{state | awaiting_setup_completition?: false} + |> Membrane.Core.LifecycleController.complete_setup() + end + |> ChildLifeController.trigger_specs() end - - @impl CallbackHandler - def handle_end_of_actions(state), do: state end diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex index 6f644947b..6fb5b745f 100644 --- a/lib/membrane/core/pipeline/state.ex +++ b/lib/membrane/core/pipeline/state.ex @@ -20,6 +20,7 @@ defmodule Membrane.Core.Pipeline.State do links: %{Link.id() => Link.t()}, crash_groups: %{CrashGroup.name() => CrashGroup.t()}, pending_specs: ChildLifeController.pending_specs(), + specs_to_trigger: [ChildLifeController.spec_ref()], synchronization: %{ timers: %{Timer.id() => Timer.t()}, clock_provider: %{ @@ -52,6 +53,7 @@ defmodule Membrane.Core.Pipeline.State do links: %{}, crash_groups: %{}, pending_specs: %{}, + specs_to_trigger: [], synchronization: nil, initialized?: false, terminating?: false, diff --git a/test/membrane/core/pipeline_test.exs b/test/membrane/core/pipeline_test.exs index 5ef8f2f7d..d59fc2b2c 100644 --- a/test/membrane/core/pipeline_test.exs +++ b/test/membrane/core/pipeline_test.exs @@ -79,6 +79,7 @@ defmodule Membrane.Core.PipelineTest do [], state ) + |> ActionHandler.handle_end_of_actions() end end @@ -92,6 +93,7 @@ defmodule Membrane.Core.PipelineTest do [], state ) + |> ActionHandler.handle_end_of_actions() end end end