Skip to content

Conversation

Watson1978
Copy link
Contributor

@Watson1978 Watson1978 commented Aug 14, 2025

Which issue(s) this PR fixes:
Fixes #5054

What this PR does / why we need it:
This PR will resolve CI error.

Docs Changes:
N/A

Release Note:
N/A

@Watson1978 Watson1978 added the CI Test/CI issues label Aug 14, 2025
@Watson1978 Watson1978 requested review from kenhys and daipom August 15, 2025 01:35
@daipom daipom added this to the v1.19.1 milestone Aug 15, 2025
Copy link
Contributor

@kenhys kenhys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Watson1978

Could you add reference to #5054 in commit message?

Except above, LGTM.

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
Even with this fix, the test still sometimes fails on my local environment.
Looks like we need the following points.

  • We should do GC.start for ObjectSpaceInputTest to ensure GC.
  • Fluent::Test.setup must ensure the initialization of Fluent::Engine.
    • It is very strange, but even if we do both remove_const and GC.start, some objects can still exist. So, it appears that we need to set nil explicitly, as your fix.

For example, how about the following fix?

diff --git a/lib/fluent/test.rb b/lib/fluent/test.rb
index ed2c3a26..6a08d5d8 100644
--- a/lib/fluent/test.rb
+++ b/lib/fluent/test.rb
@@ -40,7 +40,11 @@ module Fluent
 
       $log = dummy_logger
 
-      Fluent.__send__(:remove_const, :Engine)
+      old_engine = Fluent.__send__(:remove_const, :Engine)
+      # Ensure that GC can remove the objects of the old engine.
+      # Some objects can still exist after `remove_const`. See https://github.com/fluent/fluentd/pull/5055.
+      old_engine.instance_variable_set(:@root_agent, nil)
+
       engine = Fluent.const_set(:Engine, EngineClass.new).init(SystemConfig.new)
 
       engine.define_singleton_method(:now=) {|n|
diff --git a/test/plugin/test_in_object_space.rb b/test/plugin/test_in_object_space.rb
index 6e8bfa52..e734f398 100644
--- a/test/plugin/test_in_object_space.rb
+++ b/test/plugin/test_in_object_space.rb
@@ -21,6 +21,7 @@ class ObjectSpaceInputTest < Test::Unit::TestCase
 
   def setup
     Fluent::Test.setup
+    GC.start
     # Overriding this behavior in the global scope will have an unexpected influence on other tests.
     # So this should be overridden here and be removed in `teardown`.
     def FailObject.class

@Watson1978
Copy link
Contributor Author

hmm, I approve to add GC.start, but I wonder if we need remove_const...

This patch should fix fluent#5054

Signed-off-by: Shizuo Fujita <[email protected]>
@Watson1978
Copy link
Contributor Author

Watson1978 commented Aug 15, 2025

Could you add reference to #5054 in commit message?

Added in 4ef10d2

@daipom
Copy link
Contributor

daipom commented Aug 15, 2025

hmm, I approve to add GC.start, but I wonder if we need remove_const...

remove_const is the original code of Fluent::Test.setup.

Fluent.__send__(:remove_const, :Engine)
engine = Fluent.const_set(:Engine, EngineClass.new).init(SystemConfig.new)

What I want to say is that each test should not consider the initialization of Fluent::Engine.
It should be the responsibility of Fluent::Test.setup.

@Watson1978
Copy link
Contributor Author

What I want to say is that each test should not consider the initialization of Fluent::Engine.
It should be the responsibility of Fluent::Test.setup.

Hmmm, sounds it is not related to this PR.

@daipom
Copy link
Contributor

daipom commented Aug 18, 2025

What I want to say is that each test should not consider the initialization of Fluent::Engine.
It should be the responsibility of Fluent::Test.setup.

Hmmm, sounds it is not related to this PR.

OK. Let's separate the PRs.
I will send the PR later.

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

Copy link
Contributor

@kenhys kenhys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@daipom daipom merged commit 6cac9f0 into fluent:master Aug 18, 2025
16 of 17 checks passed
@daipom daipom added the backport to LTS We will backport this fix to the LTS branch label Aug 18, 2025
daipom added a commit that referenced this pull request Aug 18, 2025
**Which issue(s) this PR fixes**: 
Continued from

* #5054
* #5055 (6cac9f0)

**What this PR does / why we need it**: 
Each test should not consider the initialization of `Fluent::Engine`.
It should be the responsibility of `Fluent::Test.setup`.

Note: Set `nil` explicitly to ensure that GC can remove the objects,
though it is very strange that some objects can still exist after
`remove_const` and `GC.start`.

**Docs Changes**:
Not needed.

**Release Note**: 
CI improvements.

Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom daipom modified the milestones: v1.19.1, v1.20.0 Aug 21, 2025
@Watson1978 Watson1978 deleted the ci-error branch August 25, 2025 08:33
daipom added a commit to daipom/fluentd that referenced this pull request Sep 10, 2025
**Which issue(s) this PR fixes**:
Continued from

* fluent#5054
* fluent#5055 (6cac9f0)

**What this PR does / why we need it**:
Each test should not consider the initialization of `Fluent::Engine`.
It should be the responsibility of `Fluent::Test.setup`.

Note: Set `nil` explicitly to ensure that GC can remove the objects,
though it is very strange that some objects can still exist after
`remove_const` and `GC.start`.

**Docs Changes**:
Not needed.

**Release Note**:
CI improvements.

Signed-off-by: Daijiro Fukuda <[email protected]>
daipom pushed a commit to daipom/fluentd that referenced this pull request Sep 10, 2025
**Which issue(s) this PR fixes**:
Fixes fluent#5054

**What this PR does / why we need it**:
This PR will resolve CI error.

**Docs Changes**:
N/A

**Release Note**:
N/A

---------

Signed-off-by: Shizuo Fujita <[email protected]>
Signed-off-by: Daijiro Fukuda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport to LTS We will backport this fix to the LTS branch CI Test/CI issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Failed ObjectSpaceInputTest
3 participants