Skip to content

Commit

Permalink
Stopt calling handle_spec_started in between handling actions
Browse files Browse the repository at this point in the history
  • Loading branch information
FelonEkonom committed Feb 2, 2024
1 parent e323c36 commit 47e97c9
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 11 deletions.
15 changes: 14 additions & 1 deletion lib/membrane/core/bin/action_handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
2 changes: 2 additions & 0 deletions lib/membrane/core/bin/state.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -69,6 +70,7 @@ defmodule Membrane.Core.Bin.State do
links: %{},
crash_groups: %{},
pending_specs: %{},
specs_to_trigger: [],
synchronization: nil,
initialized?: false,
terminating?: false,
Expand Down
18 changes: 16 additions & 2 deletions lib/membrane/core/parent/child_life_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)

Expand Down
25 changes: 17 additions & 8 deletions lib/membrane/core/pipeline/action_handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -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
2 changes: 2 additions & 0 deletions lib/membrane/core/pipeline/state.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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: %{
Expand Down Expand Up @@ -52,6 +53,7 @@ defmodule Membrane.Core.Pipeline.State do
links: %{},
crash_groups: %{},
pending_specs: %{},
specs_to_trigger: [],
synchronization: nil,
initialized?: false,
terminating?: false,
Expand Down
2 changes: 2 additions & 0 deletions test/membrane/core/pipeline_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ defmodule Membrane.Core.PipelineTest do
[],
state
)
|> ActionHandler.handle_end_of_actions()
end
end

Expand All @@ -92,6 +93,7 @@ defmodule Membrane.Core.PipelineTest do
[],
state
)
|> ActionHandler.handle_end_of_actions()
end
end
end
Expand Down

0 comments on commit 47e97c9

Please sign in to comment.