From 5a745d46bf5ad40e8f60bb1e990648f048bf43fd Mon Sep 17 00:00:00 2001 From: Nullpointer Date: Thu, 21 Nov 2024 16:46:37 +0100 Subject: [PATCH] GD-598: Fixing memory leaks on test execution (#599) # Why Running tests via CMD line shows a lot of memory leaks at program exit. # What - fixed releasing of all singletons - `func_assert` has problems with lambdas when using inside `timer.timeout.connect`, converted lambda into function - Handling of stored asserts in the thread context results in inconsistency during cleanup - fixed cleanup on `GdUnitCommandHandlerTest` ## Finally, I have reduced most of the memory leaks nodes, but there are still `GDScriptFunctionState` orphaned nodes that I can't solve because these are handled internally by Godot see [74449](https://github.com/godotengine/godot/issues/74449) ``` Statistics: | 947 tests cases | 0 error | 0 failed | 1 flaky | 13 skipped | 0 orphans | Executed test suites: (100/104), 4 skipped Executed test cases: (934/947), 13 skipped Total time: 3min 0s 409ms Open Report at: file:///home/runner/work/gdUnit4/gdUnit4/reports/report_1/index.html Exit code: 0 ---------------------------------------------------------------- Cleanup singletons ["GdUnitThreadManager", "GdUnitDefaultValueDecoders", "GdUnitCommandHandler"] Unregister singleton 'GdUnitThreadManager' Free singleton instance 'GdUnitThreadManager:' Successfully freed 'GdUnitThreadManager' Unregister singleton 'GdUnitDefaultValueDecoders' Free singleton instance 'GdUnitDefaultValueDecoders:' Successfully freed 'GdUnitDefaultValueDecoders' Unregister singleton 'GdUnitCommandHandler' Free singleton instance 'GdUnitCommandHandler:' Successfully freed 'GdUnitCommandHandler' ---------------------------------------------------------------- Finallize .. done Finallize .. -Orphan nodes report----------------------- Finallize .. done XR: Clearing primary interface XR: Removed interface "Native mobile" XR: Removed interface "OpenXR" WARNING: ObjectDB instances leaked at exit (run with --verbose for details). at: cleanup (core/object/object.cpp:2327) Leaked instance: GDScriptFunctionState:9223373291404658414 Leaked instance: GDScriptFunctionState:92233732947[6010](https://github.com/MikeSchulze/gdUnit4/actions/runs/11956449331/job/33331231410?pr=599#step:4:6018)1646 Leaked instance: GDScriptFunctionState:9223373292545509197 Leaked instance: GDScriptFunctionState:9223373291673093970 Leaked instance: GDScriptFunctionState:9223373293719914330 Leaked instance: GDScriptFunctionState:9223373292881053539 Leaked instance: GDScriptFunctionState:9223373296521709422 Leaked instance: GDScriptFunctionState:9223378085393468557 Leaked instance: GDScriptFunctionState:9223382060754801221 Leaked instance: GDScriptFunctionState:9223373293753472331 Leaked instance: GDScriptFunctionState:9223377142497156399 Leaked instance: GDScriptFunctionState:9223378085359924447 Leaked instance: GDScriptFunctionState:9223382060788375283 Hint: Leaked instances typically happen when nodes are removed from the scene tree (with `remove_child()`) but not freed (with `free()` or `queue_free()`). Orphan StringName: test_case1 (static: 0, total: 1) Orphan StringName: _validate_callback (static: 0, total: 1) Orphan StringName: await_millis (static: 0, total: 5) Orphan StringName: test_timeout_single_yield_wait (static: 0, total: 1) Orphan StringName: test_timeout_2s (static: 0, total: 1) Orphan StringName: _execute (static: 0, total: 1) Orphan StringName: test_timeout_4s (static: 0, total: 1) Orphan StringName: timeout (static: 2, total: 8) Orphan StringName: cb_is_equal (static: 0, total: 2) Orphan StringName: test_timeout_long_running_test_abort (static: 0, total: 1) Orphan StringName: test_timeout_and_assert_fails (static: 0, total: 1) StringName: 11 unclaimed string names at exit. Run tests ends with 0 ``` --- .github/workflows/ci-pr.yml | 2 ++ .../src/asserts/GdUnitFuncAssertImpl.gd | 30 +++++++++++-------- .../src/asserts/GdUnitSignalAssertImpl.gd | 8 +++++ addons/gdUnit4/src/core/GdUnitSingleton.gd | 4 +-- .../core/execution/GdUnitExecutionContext.gd | 2 ++ .../stages/GdUnitTestCaseAfterStage.gd | 3 +- .../stages/GdUnitTestSuiteAfterStage.gd | 2 -- .../src/core/thread/GdUnitThreadContext.gd | 13 +++++--- .../core/command/GdUnitCommandHandlerTest.gd | 3 +- 9 files changed, 43 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci-pr.yml b/.github/workflows/ci-pr.yml index dcdcb020..384ecabe 100644 --- a/.github/workflows/ci-pr.yml +++ b/.github/workflows/ci-pr.yml @@ -66,6 +66,8 @@ jobs: version: installed paths: | res://addons/gdUnit4/test/ + arguments: | + --verbose timeout: 10 publish-report: false report-name: gdUnit4_Godot${{ matrix.godot-version }}-${{ matrix.godot-status }} diff --git a/addons/gdUnit4/src/asserts/GdUnitFuncAssertImpl.gd b/addons/gdUnit4/src/asserts/GdUnitFuncAssertImpl.gd index b4a8038e..270119a6 100644 --- a/addons/gdUnit4/src/asserts/GdUnitFuncAssertImpl.gd +++ b/addons/gdUnit4/src/asserts/GdUnitFuncAssertImpl.gd @@ -29,15 +29,19 @@ func _init(instance :Object, func_name :String, args := Array()) -> void: _current_value_provider = CallBackValueProvider.new(instance, func_name, args) -func _notification(_what :int) -> void: - if is_instance_valid(_current_value_provider): - _current_value_provider.dispose() - _current_value_provider = null - if is_instance_valid(_sleep_timer): - (Engine.get_main_loop() as SceneTree).root.remove_child(_sleep_timer) - _sleep_timer.stop() - _sleep_timer.free() - _sleep_timer = null +func _notification(what :int) -> void: + if what == NOTIFICATION_PREDELETE: + _interrupted = true + var main_node :Node = (Engine.get_main_loop() as SceneTree).root + if is_instance_valid(_current_value_provider): + _current_value_provider.dispose() + _current_value_provider = null + if is_instance_valid(_sleep_timer): + _sleep_timer.set_wait_time(0.0001) + _sleep_timer.stop() + main_node.remove_child(_sleep_timer) + _sleep_timer.free() + _sleep_timer = null func report_success() -> GdUnitFuncAssert: @@ -114,6 +118,10 @@ func cb_is_equal(c :Variant, e :Variant) -> bool: return GdObjects.equals(c,e) func cb_is_not_equal(c :Variant, e :Variant) -> bool: return not GdObjects.equals(c, e) +func do_interrupt() -> void: + _interrupted = true + + func _validate_callback(predicate :Callable, expected :Variant = null) -> void: if _interrupted: return @@ -125,9 +133,7 @@ func _validate_callback(predicate :Callable, expected :Variant = null) -> void: scene_tree.root.add_child(timer) timer.add_to_group("GdUnitTimers") @warning_ignore("return_value_discarded") - timer.timeout.connect(func do_interrupt() -> void: - _interrupted = true - , CONNECT_DEFERRED) + timer.timeout.connect(do_interrupt, CONNECT_DEFERRED) timer.set_one_shot(true) timer.start((_timeout/1000.0)*time_scale) _sleep_timer = Timer.new() diff --git a/addons/gdUnit4/src/asserts/GdUnitSignalAssertImpl.gd b/addons/gdUnit4/src/asserts/GdUnitSignalAssertImpl.gd index c3d5d5d6..7d4f57e4 100644 --- a/addons/gdUnit4/src/asserts/GdUnitSignalAssertImpl.gd +++ b/addons/gdUnit4/src/asserts/GdUnitSignalAssertImpl.gd @@ -22,6 +22,14 @@ func _init(emitter :Object) -> void: GdAssertReports.reset_last_error_line_number() +func _notification(what :int) -> void: + if what == NOTIFICATION_PREDELETE: + _interrupted = true + if is_instance_valid(_emitter): + _signal_collector.unregister_emitter(_emitter) + _emitter = null + + func report_success() -> GdUnitAssert: GdAssertReports.report_success() return self diff --git a/addons/gdUnit4/src/core/GdUnitSingleton.gd b/addons/gdUnit4/src/core/GdUnitSingleton.gd index 83a186f3..daa221c0 100644 --- a/addons/gdUnit4/src/core/GdUnitSingleton.gd +++ b/addons/gdUnit4/src/core/GdUnitSingleton.gd @@ -47,10 +47,10 @@ static func unregister(p_singleton :String, use_call_deferred :bool = false) -> static func dispose(use_call_deferred :bool = false) -> void: # use a copy because unregister is modify the singletons array - var singletons: PackedStringArray = Engine.get_meta(MEATA_KEY, PackedStringArray()) + var singletons :PackedStringArray = Engine.get_meta(MEATA_KEY, PackedStringArray()) GdUnitTools.prints_verbose("----------------------------------------------------------------") GdUnitTools.prints_verbose("Cleanup singletons %s" % singletons) - for singleton in singletons: + for singleton in PackedStringArray(singletons): unregister(singleton, use_call_deferred) Engine.remove_meta(MEATA_KEY) GdUnitTools.prints_verbose("----------------------------------------------------------------") diff --git a/addons/gdUnit4/src/core/execution/GdUnitExecutionContext.gd b/addons/gdUnit4/src/core/execution/GdUnitExecutionContext.gd index 904da013..0e62682e 100644 --- a/addons/gdUnit4/src/core/execution/GdUnitExecutionContext.gd +++ b/addons/gdUnit4/src/core/execution/GdUnitExecutionContext.gd @@ -348,5 +348,7 @@ func register_auto_free(obj: Variant) -> Variant: ## Runs the gdunit garbage collector to free registered object func gc() -> void: + # unreference last used assert form the test to prevent memory leaks + GdUnitThreadManager.get_current_context().clear_assert() await _memory_observer.gc() orphan_monitor_stop() diff --git a/addons/gdUnit4/src/core/execution/stages/GdUnitTestCaseAfterStage.gd b/addons/gdUnit4/src/core/execution/stages/GdUnitTestCaseAfterStage.gd index 9b78f8a6..461fb137 100644 --- a/addons/gdUnit4/src/core/execution/stages/GdUnitTestCaseAfterStage.gd +++ b/addons/gdUnit4/src/core/execution/stages/GdUnitTestCaseAfterStage.gd @@ -17,8 +17,7 @@ func _execute(context: GdUnitExecutionContext) -> void: if _call_stage: @warning_ignore("redundant_await") await test_suite.after_test() - # unreference last used assert form the test to prevent memory leaks - GdUnitThreadManager.get_current_context().set_assert(null) + await context.gc() await context.error_monitor_stop() diff --git a/addons/gdUnit4/src/core/execution/stages/GdUnitTestSuiteAfterStage.gd b/addons/gdUnit4/src/core/execution/stages/GdUnitTestSuiteAfterStage.gd index 08bf3e56..a0f8ef19 100644 --- a/addons/gdUnit4/src/core/execution/stages/GdUnitTestSuiteAfterStage.gd +++ b/addons/gdUnit4/src/core/execution/stages/GdUnitTestSuiteAfterStage.gd @@ -12,8 +12,6 @@ func _execute(context :GdUnitExecutionContext) -> void: @warning_ignore("redundant_await") await test_suite.after() - # unreference last used assert form the test to prevent memory leaks - GdUnitThreadManager.get_current_context().set_assert(null) await context.gc() var reports := context.build_reports(false) fire_event(GdUnitEvent.new()\ diff --git a/addons/gdUnit4/src/core/thread/GdUnitThreadContext.gd b/addons/gdUnit4/src/core/thread/GdUnitThreadContext.gd index 8b4ae4ff..f2b2672c 100644 --- a/addons/gdUnit4/src/core/thread/GdUnitThreadContext.gd +++ b/addons/gdUnit4/src/core/thread/GdUnitThreadContext.gd @@ -4,9 +4,9 @@ extends RefCounted var _thread :Thread var _thread_name :String var _thread_id :int -var _assert :GdUnitAssert var _signal_collector :GdUnitSignalCollector var _execution_context :GdUnitExecutionContext +var _asserts := [] func _init(thread :Thread = null) -> void: @@ -21,7 +21,7 @@ func _init(thread :Thread = null) -> void: func dispose() -> void: - _assert = null + clear_assert() if is_instance_valid(_signal_collector): _signal_collector.clear() _signal_collector = null @@ -29,12 +29,17 @@ func dispose() -> void: _thread = null +func clear_assert() -> void: + _asserts.clear() + + func set_assert(value :GdUnitAssert) -> void: - _assert = value + if value != null: + _asserts.append(value) func get_assert() -> GdUnitAssert: - return _assert + return null if _asserts.is_empty() else _asserts[-1] func set_execution_context(context :GdUnitExecutionContext) -> void: diff --git a/addons/gdUnit4/test/core/command/GdUnitCommandHandlerTest.gd b/addons/gdUnit4/test/core/command/GdUnitCommandHandlerTest.gd index 8870784b..9500ecc2 100644 --- a/addons/gdUnit4/test/core/command/GdUnitCommandHandlerTest.gd +++ b/addons/gdUnit4/test/core/command/GdUnitCommandHandlerTest.gd @@ -15,8 +15,7 @@ func before() -> void: func after() -> void: - _handler._notification(NOTIFICATION_PREDELETE) - _handler = null + _handler.free() @warning_ignore('unused_parameter')