Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 35 commits into from
Feb 16, 2023
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
46d1df4
Add `handle_child_pad_removed/4` callback in bins and pipelines
FelonEkonom Jan 18, 2023
81cc84d
Update changelog
FelonEkonom Jan 19, 2023
fa27e4b
Fix credo issue
FelonEkonom Jan 19, 2023
df5bd3e
Fix dialyzer issues
FelonEkonom Jan 19, 2023
54f8a4e
Write tests for handle_child_pad_removed
FelonEkonom Jan 24, 2023
835cac6
Send :handle_unlink, even if child is terminating
FelonEkonom Jan 25, 2023
59d8892
Merge remote-tracking branch 'origin/master' into handle-child-pad-re…
FelonEkonom Jan 25, 2023
cf7a11c
Refactor handle_child_pad_removed callback related code
FelonEkonom Jan 25, 2023
818ef92
Stop raising, on unliunking static pad, when component is terminating…
FelonEkonom Jan 25, 2023
7860906
Remove leftovers from handle_child_pad_removed tests
FelonEkonom Jan 25, 2023
dedb30b
Merge remote-tracking branch 'origin/master' into handle-child-pad-re…
FelonEkonom Jan 26, 2023
f346161
Make dependent specs a map
FelonEkonom Jan 26, 2023
270fb9d
Remove link from specs, when child removes it's pad
FelonEkonom Jan 26, 2023
c1fd3bc
Update specs, when parent removes a child
FelonEkonom Jan 27, 2023
b111be3
Fix bug in Testing.Pipeline
FelonEkonom Jan 27, 2023
8b53257
Fix bug in pipeline termination & write tests for killing elements wh…
FelonEkonom Jan 30, 2023
c2112be
Refactor :handle_unlink message
FelonEkonom Jan 30, 2023
999c59f
Implement hanlde_child_pad_removed in Testing.Pipeline
FelonEkonom Jan 31, 2023
36a10dd
Fix failing Testing.Pipeline tests
FelonEkonom Jan 31, 2023
5624599
Fix typo
FelonEkonom Jan 31, 2023
94b0cad
Update lib/membrane/testing/assertions.ex
FelonEkonom Feb 2, 2023
89deeff
Implement suggestion from CR
FelonEkonom Feb 2, 2023
5299101
Merge branch 'handle-child-pad-removed-bug-fix' of github.com:membran…
FelonEkonom Feb 2, 2023
a6ee5f2
Remove done todo annotation
FelonEkonom Feb 2, 2023
df960e3
Run format
FelonEkonom Feb 2, 2023
703fa5a
Remove commented lines of code
FelonEkonom Feb 2, 2023
16a6832
Rename function remove_dynamic_pad! to remove_pad
FelonEkonom Feb 2, 2023
28f1644
Refactor removing children and links, while specs are still proceeded
FelonEkonom Feb 7, 2023
f4de5d3
Refactor handle_link error handling
FelonEkonom Feb 13, 2023
73e8501
Replace MapSet.reject/2 with Enum.reject/2 and MapSet.new/1
FelonEkonom Feb 13, 2023
214978c
Merge remote-tracking branch 'origin/master' into handle-child-pad-re…
FelonEkonom Feb 14, 2023
7e4577e
Refactor ChildLifeController due to code review
FelonEkonom Feb 14, 2023
ad72e26
Implement suggestions from code review
FelonEkonom Feb 15, 2023
62f6647
Merge remote-tracking branch 'origin/master' into handle-child-pad-re…
FelonEkonom Feb 15, 2023
ce0fb4c
Fix invalid typespec
FelonEkonom Feb 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* 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)
* 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)

## 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)
Expand Down
20 changes: 17 additions & 3 deletions lib/membrane/bin.ex
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@ defmodule Membrane.Bin do
) ::
callback_return

@doc """
Callback invoked when a child removes its pad.

The callback won't be invoked, when you have initiated the pad removal,
eg. when you have returned `t:Membrane.Bin.Action.remove_link()` action
which made one of your children's pads be removed.
"""
@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.
"""
Expand Down Expand Up @@ -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,
Expand All @@ -183,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
Expand Down
5 changes: 5 additions & 0 deletions lib/membrane/core/bin.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
132 changes: 85 additions & 47 deletions lib/membrane/core/bin/pad_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,22 @@ defmodule Membrane.Core.Bin.PadController do
state
end

@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])
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"
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
Expand Down Expand Up @@ -193,58 +209,80 @@ 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}
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)

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]
)
end

params =
Map.update!(
params,
:stream_format_validation_params,
&[{state.module, pad_name} | &1]
)
handle_link_response =
Message.call(child_endpoint.pid, :handle_link, [
direction,
child_endpoint,
other_endpoint,
params
])

case handle_link_response do
{:error, {:call_failure, reason}} ->
Membrane.Logger.debug("""
Tried to link pad #{inspect(endpoint.pad_ref)}, but child #{inspect(endpoint.child)},
that was supposed to be linked to this pad, is not alive.
""")

{{:error, {:child_dead, reason}}, state}

{:error, _reason} = error ->
{error, state}

reply ->
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}
end
else
{:error, :unknown_pad} ->
mat-hek marked this conversation as resolved.
Show resolved Hide resolved
Membrane.Logger.debug("""
Tried to link pad #{inspect(endpoint.pad_ref)}, but this pad is not existing
in the moment of receiving :handle_link. It could be removed or never existed.
""")

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}
{{:error, {:unknown_pad, state.name, endpoint.pad_ref}}, state}
end
end

@doc """
Expand Down
11 changes: 11 additions & 0 deletions lib/membrane/core/element/pad_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,18 @@ defmodule Membrane.Core.Element.PadController do
{:ok, state}

{:error, {:call_failure, reason}} ->
Membrane.Logger.debug("""
Tried to link pad #{inspect(endpoint.pad_ref)}, but neighbour #{inspect(other_endpoint.child)}
is not alive.
""")

{{:error, {:neighbor_dead, reason}}, state}

{:error, {:unknown_pad, _name, _pad_ref}} = error ->
{error, state}

{:error, {:child_dead, reason}} ->
{{:error, {:neighbor_child_dead, reason}}, state}
end
end

Expand Down
Loading