Skip to content

Remove SingleCollector_CollectWordRunning#6426

Open
fingolfin wants to merge 1 commit into
masterfrom
mh/remove-SingleCollector_CollectWordRunning
Open

Remove SingleCollector_CollectWordRunning#6426
fingolfin wants to merge 1 commit into
masterfrom
mh/remove-SingleCollector_CollectWordRunning

Conversation

@fingolfin
Copy link
Copy Markdown
Member

It does not seem to serve any purpose (anymore?)

Resolves #6412

Though perhaps @hulpke or @ThomasBreuer know / remember what this was about better than me?

It does not seem to serve any purpose (anymore?)
@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jun 5, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.86%. Comparing base (d0fd036) to head (184512f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6426      +/-   ##
==========================================
- Coverage   78.86%   78.86%   -0.01%     
==========================================
  Files         685      685              
  Lines      293551   293543       -8     
  Branches     8672     8672              
==========================================
- Hits       231521   231513       -8     
+ Misses      60224    60222       -2     
- Partials     1806     1808       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

This is clearly a safety catch to avoid collection code calling the collector again, which might happen when working on implementing a new collector. Why should it go? (It might not be the best kind of implementation, but that is a different question)

@fingolfin
Copy link
Copy Markdown
Member Author

@hulpke could you clarify what exactly it would protect against? I understand that it is meant to protect against recursion issues, but I don't understand what issues those would be? There used to be code in the kernel collector that was not re-entrant, because it used a global shared buffer. But this one doesn't do that, as far as I can tell (I may have missed something).

The motivation for removing it is that the flag is not cleared when exiting via a break loop, which can lead to confusing errors for the user (see issue #6412).

@hulpke
Copy link
Copy Markdown
Contributor

hulpke commented Jun 6, 2026

I can think of two situations. basically both having to do with implementing new collectors: The first is to have the new code call an old collection for verification in a test. The second would be a mixed collection that uses the old collection for a subgroup.
In both cases this could cause strange problems. But if the non-reentrant is not an issue any more, no problem from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error, collector is not reentrant after previously interrupting code.

3 participants