Update processed specs, when children or link is down #517

FelonEkonom · 2023-01-26T17:35:48Z

Rekated Jira ticket: https://membraneframework.atlassian.net/browse/MC-148

…moved

…, but it's spec is not ready yet

…moved-bug-fix

…ile specs are processed

mat-hek · 2023-02-06T09:58:49Z

lib/membrane/pipeline.ex

+  @doc """
+  Callback invoked when a child removes its pad.
+
+  Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()`


this whole sentence is hard to understand anyway, let's rephrase it

mat-hek · 2023-02-06T10:06:21Z

lib/membrane/core/parent/child_life_controller.ex

    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)


is there any point of proceeding with a spec startup when it's not pending?

if so, we can merge the case and with into a single with

lib/membrane/core/bin/pad_controller.ex

mat-hek · 2023-02-06T16:42:46Z

lib/membrane/core/parent/child_life_controller.ex

+      |> MapSet.new(& &1.id)
+
+    affected_specs =
+      state.pending_specs
+      |> Enum.filter(fn {_ref, spec_data} ->
+        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 =
+      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()
+
+        dependencies =
+          spec_data.dependencies
+          |> update_spec_dependencies(children_set)
+
+        spec_data = %{
+          spec_data
+          | children_names: children_names,
+            links_ids: links_ids,
+            awaiting_responses: awaiting_responses,
+            dependencies: dependencies
+        }
+
+        {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)
+
+      dependencies =
+        [removed_link.from.child, removed_link.to.child]
+        |> Enum.filter(&(&1 not in spec_links_endpoints))
+        |> case do
+          [] ->
+            spec_data.dependencies
+
+          endpoints_to_remove ->
+            spec_data.dependencies
+            |> update_spec_dependencies(endpoints_to_remove)
+        end
+
+      awaiting_responses =
+        spec_data.awaiting_responses
+        |> MapSet.difference(MapSet.new([{link_id, :input}, {link_id, :output}]))
+
+      spec_data = %{
+        spec_data
+        | dependencies: dependencies,
+          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
+
+  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


don't proceed with a dependent spec even if the dependency is removed

simplify the code by iterating through children first and making use of spec_ref in child and link data

…moved-bug-fix

mat-hek

TBC

lib/membrane/core/parent/child_life_controller.ex

mat-hek · 2023-02-14T14:28:30Z

lib/membrane/core/parent/child_life_controller.ex

+      state.pending_specs
+      |> Enum.filter(fn {_ref, spec_data} ->
+        not MapSet.disjoint?(spec_data.children_names, removed_children) or
+          not MapSet.disjoint?(spec_data.links_ids, removed_links_ids)
+      end)


we can get links specs as stated in the comment above and children specs from children data, so that this won't be needed

mat-hek · 2023-02-14T14:29:26Z

lib/membrane/core/parent/child_life_controller.ex

+    state = Map.update!(state, :pending_specs, &Map.merge(&1, updated_specs))
+
+    updated_specs
+    |> Enum.map(fn {spec_ref, _spec_data} -> spec_ref end)


Suggested change

|> Enum.map(fn {spec_ref, _spec_data} -> spec_ref end)

|> Map.keys()

mat-hek · 2023-02-14T14:52:42Z

lib/membrane/core/parent/child_life_controller.ex

+  def remove_link_from_specs(link_id, state) when is_reference(link_id) do
+    link = Map.fetch!(state.links, link_id)
+    spec_ref = link.spec_ref
+
+    with %{pending_specs: %{^spec_ref => spec_data}} <- state do
+      links_ids = MapSet.delete(spec_data.links_ids, link.id)
+
+      awaiting_responses =
+        spec_data.awaiting_responses
+        |> Enum.reject(&match?({^link_id, _direction}, &1))
+        |> MapSet.new()
+
+      state =
+        put_in(
+          state,
+          [:pending_specs, spec_ref],
+          %{spec_data | links_ids: links_ids, awaiting_responses: awaiting_responses}
+        )
+
+      proceed_spec_startup(spec_ref, state)


It still duplicates the code from remove_children_from_specs

mat-hek · 2023-02-14T14:54:28Z

lib/membrane/core/parent/child_life_controller.ex

-    dependent_specs =
+    dependencies =
      state.pending_specs
-      |> Enum.filter(fn {_ref, data} -> spec_ref in data.dependent_specs end)
+      |> Enum.filter(fn {_ref, data} -> MapSet.member?(data.dependent_specs, spec_ref) end)
      |> Map.new(fn {ref, data} ->
        {ref, Map.update!(data, :dependent_specs, &MapSet.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)


seems these are dependent_specs again

mat-hek · 2023-02-15T10:59:40Z

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


the above can now be is_static_pad_ref or is_dynamic_pad_ref

mat-hek · 2023-02-15T11:21:03Z

lib/membrane/pipeline.ex

+  @doc """
+  Callback invoked when a child removes its pad.
+
+  Removing child's pad due to return `t:Membrane.Bin.Action.remove_link()`


I'm not sure if this changed, but I still have a hard time understanding this. The doc for this callback in Bin is even more sophisticated since there is a 'parent' and 'child' bin involved and it's hard to determine which is which. Maybe let's focus on when this callback is called and provide some example/s.

mat-hek · 2023-02-15T11:36:24Z

lib/membrane/testing/pipeline.ex

+      Child #{inspect(child)} removed it's pad #{inspect(pad)}. If you want 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.


name option?

…moved-bug-fix

This reverts commit 924aec4.

FelonEkonom added 12 commits January 19, 2023 16:48

Add handle_child_pad_removed/4 callback in bins and pipelines

46d1df4

Update changelog

81cc84d

Fix credo issue

fa27e4b

Fix dialyzer issues

df5bd3e

Write tests for handle_child_pad_removed

54f8a4e

Send :handle_unlink, even if child is terminating

835cac6

Merge remote-tracking branch 'origin/master' into handle-child-pad-re…

59d8892

…moved

Refactor handle_child_pad_removed callback related code

cf7a11c

Stop raising, on unliunking static pad, when component is terminating…

818ef92

…, but it's spec is not ready yet

Remove leftovers from handle_child_pad_removed tests

7860906

Merge remote-tracking branch 'origin/master' into handle-child-pad-re…

dedb30b

…moved-bug-fix

Make dependent specs a map

f346161

FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Jan 26, 2023

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch 2 times, most recently from 842186b to c5eec27 Compare January 26, 2023 18:08

Remove link from specs, when child removes it's pad

270fb9d

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch from bc340e4 to 270fb9d Compare January 27, 2023 12:35

FelonEkonom added 3 commits January 27, 2023 14:30

Update specs, when parent removes a child

c1fd3bc

Fix bug in Testing.Pipeline

b111be3

Fix bug in pipeline termination & write tests for killing elements wh…

8b53257

…ile specs are processed

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch from 326f10a to 8b53257 Compare January 30, 2023 12:25

FelonEkonom marked this pull request as ready for review January 30, 2023 12:50

FelonEkonom requested a review from mat-hek as a code owner January 30, 2023 12:51

FelonEkonom marked this pull request as draft January 30, 2023 13:03

Refactor :handle_unlink message

c2112be

FelonEkonom marked this pull request as ready for review January 30, 2023 13:14

FelonEkonom added 3 commits January 31, 2023 14:32

Implement hanlde_child_pad_removed in Testing.Pipeline

999c59f

Fix failing Testing.Pipeline tests

36a10dd

Fix typo

5624599

FelonEkonom requested a review from varsill February 1, 2023 12:39

FelonEkonom removed the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Feb 2, 2023

mat-hek requested changes Feb 6, 2023

View reviewed changes

Refactor removing children and links, while specs are still proceeded

28f1644

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch from 647747e to 28f1644 Compare February 7, 2023 15:13

Refactor handle_link error handling

f4de5d3

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch from e8ae474 to f4de5d3 Compare February 13, 2023 12:55

Replace MapSet.reject/2 with Enum.reject/2 and MapSet.new/1

73e8501

FelonEkonom requested a review from mat-hek February 13, 2023 13:15

Merge remote-tracking branch 'origin/master' into handle-child-pad-re…

214978c

…moved-bug-fix

mat-hek requested changes Feb 14, 2023

View reviewed changes

Refactor ChildLifeController due to code review

7e4577e

FelonEkonom requested a review from mat-hek February 14, 2023 17:44

mat-hek requested changes Feb 15, 2023

View reviewed changes

FelonEkonom requested a review from mat-hek February 15, 2023 12:48

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch 2 times, most recently from fc820bf to 515984d Compare February 15, 2023 13:22

Implement suggestions from code review

ad72e26

FelonEkonom force-pushed the handle-child-pad-removed-bug-fix branch from 515984d to ad72e26 Compare February 15, 2023 13:33

Merge remote-tracking branch 'origin/master' into handle-child-pad-re…

62f6647

…moved-bug-fix

mat-hek approved these changes Feb 15, 2023

View reviewed changes

FelonEkonom requested review from varsill and removed request for varsill February 15, 2023 17:20

Fix invalid typespec

ce0fb4c

varsill approved these changes Feb 15, 2023

View reviewed changes

FelonEkonom merged commit 924aec4 into master Feb 16, 2023

FelonEkonom deleted the handle-child-pad-removed-bug-fix branch February 16, 2023 10:59

FelonEkonom added a commit that referenced this pull request Feb 16, 2023

Revert "Update processed specs, when children or link is down (#517)"

4d5c060

This reverts commit 924aec4.

FelonEkonom mentioned this pull request Feb 16, 2023

Revert "Update processed specs, when children or link is down" #529

Closed

FelonEkonom restored the handle-child-pad-removed-bug-fix branch February 16, 2023 13:07

FelonEkonom deleted the handle-child-pad-removed-bug-fix branch February 16, 2023 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update processed specs, when children or link is down #517

Update processed specs, when children or link is down #517

FelonEkonom commented Jan 26, 2023

mat-hek Feb 6, 2023

mat-hek Feb 6, 2023

mat-hek Feb 6, 2023

mat-hek left a comment

mat-hek Feb 14, 2023

mat-hek Feb 14, 2023

mat-hek Feb 14, 2023

mat-hek Feb 14, 2023

mat-hek Feb 15, 2023

mat-hek Feb 15, 2023

mat-hek Feb 15, 2023

	\|> Enum.map(fn {spec_ref, _spec_data} -> spec_ref end)
	\|> Map.keys()

		@@ -164,6 +164,12 @@ defmodule Membrane.Pad do
		(term \|> is_tuple and term \|> tuple_size == 3 and term \|> elem(0) == __MODULE__ and

Update processed specs, when children or link is down #517

Update processed specs, when children or link is down #517

Conversation

FelonEkonom commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment