Per #5024 (comment), there's a very high likelihood that the stopgap proposed here wouldn't be necessary. So I'd suggest to hold off on merging -- I'm guessing >90% chance of closing this one without a merge.

Per #5024 (comment), there's a very high likelihood that the stopgap proposed here wouldn't be necessary. So I'd suggest to hold off on merging -- I'm guessing >90% chance of closing this one without a merge.

Per my subsequent comment there, your proposed alternative breaks stop on Pmono, alas.

The two-part approach to Pmono cleanups is alas not looking like it can be avoided, given that downstream filters of Pmono events, including the EventStreamPlayer, have a "chicken and egg" problem with Pmono cleanups: they need to register them, but before the id is known... because the event hasn't played yet.

I know I suggested a simple closure-based solution to Pmono's cleanup, but alas it looks not feasible within the constraints of how only events are used to propagate cleanups "downstream". There's no general callback mechanism in which all streams which may want to get a new cleanup can be notified to add one.. from a callback from Event. For a closure solution to work, we'd need a broadcast version of updatePmono in which every stream (or player) that depends on that Pmono will get notified to add a cleanup to their own copy of the cleanups, after the monoNote event has played. I suppose cleanup-aware streams could do something like that using addDependant as a hack implemented in EventStreamCleanup.update, which would do that addDependant when it sees updatePmono in the event (or more generally a new dependency flag/mechanisms like that that's not particular to Pmono), but it seems a non-trivial solution that would require a fair bit of testing. (And frankly registering a possibly no-op cleanup, as we do now, doesn't look worse than registering a dependency so that a stream can get notified when a new real cleanup is actually added from an Event callback.)

So, I will be basing my PR for the general cleanup dedupe assuming this solution (5027) is in place, simply because it's a minimalist fix to the problem introduced/exposed in #4787.

FWIW, I just verified that the problem is not reproducible after merging #5030, with no other changes (and stopping a regular Pmono is not broken with #5030 in place) -- unsurprising as 5030 seems to incorporate the change here.

I'll wait a bit for other feedback, but AFAICS this PR doesn't need to be merged.

unsurprising as 5030 seems to incorporate the change here

That was intentional because of (1) the potential edit conflict since they changed adjacent lines of code... and (2) I wanted to check that they work together.

However, 5030 might take substantially longer to review/approve/merge.

@eleses @jamshark70 should we close this then?

Hm... "unsurprising as 5030 seems to incorporate the change here."

So if we keep it in 5030, then this PR would be redundant. However 5030 seems to include changes not related to CleanupThunk/CallOnce. It might be worth narrowing the scope. Actually I looked again -- apart from the discussion over the name, 5030's changes all seem germane.

I suspect we want to keep it -- a cleanup event with a nil ID is not good, regardless of how we got there.

@jamshark70 and @dyfer - OK to close?

I confirmed that the issue is still reproducible in current develop, but it is not reproducible after applying #5386 (de-dup EventStreamCleanup). This was a fortunate accident -- I had applied 5386 locally to check the behavior of CallOnce().valueArray, and forgotten to switch back to develop before checking this case. It worked fine with 5386.

So, if we have a commitment to merge #5386, then this one can be closed.