Clear canceled task node early

[gaearon]

This is unobservable. There should be no need to repeatedly clear it if we already did that once.

[sizebot]

No significant bundle size changes to report.

Generated by 🚫 dangerJS

[gaearon]

Note we should probably wait before merging because this may mask the bug we found in #16145.

[sebmarkbage]

Why is this change needed? Seems like it would serve to cover up another bug.

[gaearon]

Not strictly needed, but it's kind of inefficient? This fires on every discrete event and @acdlite's PR adds more potential work to "cancel". Unless you're saying we should keep cancel close to free instead.

[gaearon]

I wouldn't also say it necessarily covers things up. The way it exposed the bug was already super convoluted and hard to follow.

The proper way to expose it early is by having tests for cancelCallback (we didn't have any). I added a couple already, and we can add more. But we probably shouldn't rely on this as primary way to expose them because it's too deep in the stack.

[sebmarkbage]

There are more things that are awkwardly structured with this value. What is the life-time before it gets reset to null? Seemly infinitely since it doesn't get reset when it runs.

What happens if you call scheduleSyncCallback but there already is one scheduled? Then there's no way to cancel the first one.

Seems like this is a leaky abstraction that has a lot of assumptions about how it's going to get called which might indicate that pretending that this is the same API as the public scheduler API isn't the right abstraction here.

[gaearon]

OK that is a bigger sink that I thought at first. Want to revert this? I find it confusing either way.

[sebmarkbage]

Don't want to revert it. Just understand where the wish to change this came from since it can reveal further issues (like that the API contract is fragile to begin with).