Modify 5s linking timeout constraint #810
Conversation
FelonEkonom
added
the
no-changelog
FelonEkonom
force-pushed
the
modify_5s_linking_constraint
branch
from
453de6e
to
814f10b
Compare
| "Bin pad #{inspect(pad_ref)} wasn't linked internally within timeout. Pad data: #{inspect(pad_data, pretty: true)}" | ||
| case Map.fetch(state.linking_timeout_counters, pad_ref) do | ||
| {:ok, 1} -> | ||
| Map.update!(state, :linking_timeout_counters, &Map.delete(&1, pad_ref)) |
There was a problem hiding this comment.
Remember we talked about something similar, but couldn't we store the reference to the timer in pad_data and compare it when the timeout arrives?
There was a problem hiding this comment.
It is not a good idea, this approach generates some weird race conditions and it's implementation would be far more complex. I don't se any pros of it, only cons
There was a problem hiding this comment.
Conclusion from our last talk was that we don't want to do this
| @@ -353,6 +353,7 @@ defmodule Membrane.Integration.LinkingTest do | |||
| Membrane.Pipeline.terminate(pipeline) | |||
| end | |||
|
|
|||
| @tag :xd | |||
| with {:error, :unknown_pad} <- PadModel.assert_instance(state, pad_ref) do | ||
| PadController.init_pad_data(pad_ref, state) | ||
| else | ||
| :ok -> state | ||
| end |
| @@ -50,8 +49,7 @@ defmodule Membrane.Core.Bin.PadController do | |||
| state = | |||
| case PadModel.get_data(state, pad_ref) do | |||
| {:error, :unknown_pad} -> | |||
| init_pad_data(pad_ref, pad_info, state) | |||
| |> Map.update!(:pad_refs, &[pad_ref | &1]) | |||
There was a problem hiding this comment.
Why remove pad_refs? In case you forgot, it was added by yourself in this PR: #641 :P
There was a problem hiding this comment.
Ok, I didn't remember it 🙈 There was 1 place, where the pad was removed from pads_data but not from pad_refs, so without this PR value stored under this field may be incorrec. I saw that nothing relies on the value of this field. I guess the reason of this field was to have more readable logs, but has anybody ever looked at it in the logs to debug something?
There was a problem hiding this comment.
I didn't, but hard to say in general. I have the following idea:
- call it
inspect_pad_refsand assign to it a function that will accept the state and return the pad refs - in
Membrane.Core.Inspect, if a field starts withinspect_, we'll call the function in it instead of printing it directly
I don't like it much, but should be better than what we have. Otherwise I'd just remove this field, but it should be removed from the element as well
There was a problem hiding this comment.
there is no such field in the element state
Solves #786