Remove SingleCollector_CollectWordRunning

[fingolfin]

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?

[codecov]

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.

[hulpke]

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]

@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]

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.

[fingolfin]

Thanks for taking the time to explain, @hulpke . Unfortunately I still don't understand (despite having written a mixed collector myself in the past):

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.

That'd be a fair use case -- but how could this lead to the old collector being entered twice? I can see how the new collector might want to have some re-entrance protection here, to make sure it doesn't accidentally call itself for verification (although I expect that'd lead to infinite recursion and an error anyway, so might not be necessary to have a manual check in place after all)

The second would be a mixed collection that uses the old collection for a subgroup.

Again, I fail to see how the old collector could be re-entered in that scenario?

[hulpke]

That'd be a fair use case -- but how could this lead to the old collector being entered twice?

Because someone implements it in a weird way. I agree that it should not be an issue. But I have seen too many weird things done in code to assume nobody would do this -- trapping an entry to non-reentrant code looks to me as a per-se good practice (s any error would be horrible to debug).

[fingolfin]

But the SingleCollector code is in fact re-entrant.