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

fix: validate that the "old" part of a reconfigured pair will get garbage collected #358

Open
wants to merge 4 commits into
base: transition-to-runkit
Choose a base branch
from
Open
Changes from 1 commit
Commits
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
Next Next commit
fix: handle task.depends_on(composition) when scheduling reconfigurat…
…ions

While the normal data structure does not generate this type of patterns,
they must be supported - it is generally speaking useful, and is already
used by e.g. the transformer to represent dynamic transformation producers.

The pattern obviously already triggered a bug since there was a specific
bug fix for a task context->task context dependency. The fix was however
too limited as it would not handle task context->composition.

This change implements a pass after deployment where we would find the
root components from the "old plan" that are not used in the "new plan",
and cut the dependency relations between these two. This should really
handle all cases.
doudou committed Apr 11, 2023
commit 92d74cd2fa97acba0fa9c1b28a6e69094cae96d6
50 changes: 39 additions & 11 deletions lib/syskit/network_generation/engine.rb
Original file line number Diff line number Diff line change
@@ -137,6 +137,8 @@ def apply_deployed_network_to_plan
finalize_deployed_tasks
end

sever_old_plan_from_new_plan

if @dataflow_dynamics
@dataflow_dynamics.apply_merges(merge_solver)
log_timepoint "apply_merged_to_dataflow_dynamics"
@@ -147,6 +149,42 @@ def apply_deployed_network_to_plan
end
end

# "Cut" relations between the "old" plan and the new one
#
# At this stage, old components (task contexts and compositions)
# that are not part of the new plan may still be child of bits of
# the new plan. This happens if they are added as children of other
# task contexts. The transformer does this to register dynamic
# transformation producers
#
# This pass looks for all proxies of compositions and task contexts
# that are not the target of a merge operation. When this happens,
# we know that the component is not being reused, and we remove all
# dependency relations where it is child and where the parent is
# "useful"
#
# Note that we do this only for relations between Syskit
# components. Relations with "plan" Roby tasks are updated because
# we replace toplevel tasks.
def sever_old_plan_from_new_plan
old_tasks =
work_plan
.find_local_tasks(Syskit::Component)
.find_all(&:transaction_proxy?)

merge_leaves = merge_solver.each_merge_leaf.to_set
old_tasks.each do |old_task|
next if merge_leaves.include?(old_task)

parents =
old_task
.each_parent_task
.find_all { |t| merge_leaves.include?(t) }

parents.each { |t| t.remove_child(old_task) }
end
end

class << self
# Set of blocks registered with
# register_instanciation_postprocessing
@@ -602,9 +640,7 @@ def adapt_existing_deployment(deployment_task, existing_deployment_task)
deployed_tasks.each do |task|
existing_tasks =
orocos_name_to_existing[task.orocos_name] || []
unless existing_tasks.empty?
existing_task = find_current_deployed_task(existing_tasks)
end
existing_task = find_current_deployed_task(existing_tasks)

if !existing_task || !task.can_be_deployed_by?(existing_task)
debug do
@@ -629,14 +665,6 @@ def adapt_existing_deployment(deployment_task, existing_deployment_task)
"to finish before reconfiguring"
end

parent_task_contexts =
previous_task
.each_parent_task
.find_all { |t| t.kind_of?(Syskit::TaskContext) }

parent_task_contexts.each do |t|
t.remove_child(previous_task)
end
new_task.should_configure_after(previous_task.stop_event)
end
existing_task = new_task
8 changes: 8 additions & 0 deletions lib/syskit/network_generation/merge_solver.rb
Original file line number Diff line number Diff line change
@@ -572,6 +572,14 @@ def display_merge_graph(title, merge_graph)
break
end
end

def each_merge_leaf
return enum_for(__method__) unless block_given?

task_replacement_graph.each_vertex do |v|
yield(v) if task_replacement_graph.leaf?(v)
end
end
end
end
end
148 changes: 93 additions & 55 deletions test/network_generation/test_engine.rb
Original file line number Diff line number Diff line change
@@ -195,61 +195,6 @@ def work_plan
.with_arguments(orocos_name: task.orocos_name).to_a
assert_equal work_plan.wrap([task]), tasks
end

describe "when child of a composition" do
it "ensures that the existing deployment will be garbage collected" do
task_m = Syskit::TaskContext.new_submodel
cmp_m = Syskit::Composition.new_submodel
cmp_m.add task_m, as: "test"

syskit_stub_configured_deployment(task_m)
cmp = syskit_deploy(cmp_m)
original_task = cmp.test_child
flexmock(task_m).new_instances.should_receive(:can_be_deployed_by?)
.with(->(proxy) { proxy.__getobj__ == cmp.test_child }).and_return(false)
new_cmp = syskit_deploy(cmp_m)

# Should have instanciated a new composition since the children
# differ
refute_equal new_cmp, cmp
# Should have of course created a new task
refute_equal new_cmp.test_child, cmp.test_child
# And the old tasks should be ready to garbage-collect
assert_equal [cmp, original_task].to_set,
execute { plan.static_garbage_collect.to_set }
end
end

describe "when child of a task" do
it "ensures that the existing deployment will be garbage collected" do
child_m = Syskit::TaskContext.new_submodel
parent_m = Syskit::TaskContext.new_submodel
parent_m.singleton_class.class_eval do
define_method(:instanciate) do |*args, **kw|
task = super(*args, **kw)
task.depends_on(child_m.instanciate(*args, **kw),
role: "test")
task
end
end

syskit_stub_configured_deployment(child_m)
parent_m = syskit_stub_requirements(parent_m)
parent = syskit_deploy(parent_m)
child = parent.test_child

flexmock(child_m).new_instances.should_receive(:can_be_deployed_by?)
.with(->(proxy) { proxy.__getobj__ == child }).and_return(false)
new_parent = syskit_deploy(parent_m)
new_child = new_parent.test_child

assert_equal new_parent, parent
refute_equal new_child, child
# And the old tasks should be ready to garbage-collect
assert_equal [child].to_set,
execute { plan.static_garbage_collect.to_set }
end
end
end

describe "#adapt_existing_deployment" do
@@ -343,6 +288,99 @@ def work_plan
end
end

describe "when scheduling tasks for reconfiguration" do
it "ensures that the old task is gargabe collected "\
"when child of a composition" do
task_m = Syskit::TaskContext.new_submodel
cmp_m = Syskit::Composition.new_submodel
cmp_m.add task_m, as: "test"

syskit_stub_configured_deployment(task_m)
cmp = syskit_deploy(cmp_m)
original_task = cmp.test_child
flexmock(task_m).new_instances.should_receive(:can_be_deployed_by?)
.with(->(proxy) { proxy.__getobj__ == cmp.test_child }).and_return(false)
new_cmp = syskit_deploy(cmp_m)

# Should have instanciated a new composition since the children
# differ
refute_equal new_cmp, cmp
# Should have of course created a new task
refute_equal new_cmp.test_child, cmp.test_child
# And the old tasks should be ready to garbage-collect
assert_equal [cmp, original_task].to_set,
execute { plan.static_garbage_collect.to_set }
end

it "ensures that the old task gets garbage collected when child "\
"of another still useful task" do
child_m = Syskit::TaskContext.new_submodel
parent_m = Syskit::TaskContext.new_submodel
parent_m.singleton_class.class_eval do
define_method(:instanciate) do |*args, **kw|
task = super(*args, **kw)
task.depends_on(child_m.instanciate(*args, **kw),
role: "test")
task
end
end

syskit_stub_configured_deployment(child_m)
parent_m = syskit_stub_requirements(parent_m)
parent = syskit_deploy(parent_m)
child = parent.test_child

flexmock(child_m)
.new_instances.should_receive(:can_be_deployed_by?)
.with(->(proxy) { proxy.__getobj__ == child }).and_return(false)
new_parent = syskit_deploy(parent_m)
new_child = new_parent.test_child

assert_equal new_parent, parent
refute_equal new_child, child
# And the old tasks should be ready to garbage-collect
assert_equal [child].to_set,
execute { plan.static_garbage_collect.to_set }
end

it "ensures that the old task gets garbage collected when child "\
"of a composition, itself child of a useful task" do
child_m = Syskit::TaskContext.new_submodel
cmp_m = Syskit::Composition.new_submodel
cmp_m.add child_m, as: "task"
parent_m = Syskit::TaskContext.new_submodel
parent_m.singleton_class.class_eval do
define_method(:instanciate) do |*args, **kw|
task = super(*args, **kw)
task.depends_on(cmp_m.instanciate(*args, **kw),
role: "test")
task
end
end

syskit_stub_configured_deployment(child_m)
parent_m = syskit_stub_requirements(parent_m)
parent = syskit_deploy(parent_m)
child = parent.test_child
child_task = child.task_child

flexmock(child_m)
.new_instances.should_receive(:can_be_deployed_by?)
.with(->(proxy) { proxy.__getobj__ == child_task })
.and_return(false)
new_parent = syskit_deploy(parent_m)
new_child = new_parent.test_child
new_child_task = new_child.task_child

assert_equal new_parent, parent
refute_equal new_child, child
refute_equal new_child_task, child_task
# And the old tasks should be ready to garbage-collect
assert_equal [child, child_task].to_set,
execute { plan.static_garbage_collect.to_set }
end
end

describe "#find_current_deployed_task" do
it "ignores garbage tasks that have not been finalized yet" do
component_m = Syskit::Component.new_submodel