Cancellable: set token from node/async in features code

[auduchinok]

Rewrites parts of Cancellable.CheckAndThrow. In the original implementation the token was set inside Cancellable.run, which made it work automatically and on a lower level. However, it turned out to be problematic in testing it in more real-world situations, as not all of the FCS checking related code is wrapped in cancellable expressions.

The original implementation maintained list of tokens, which allowed scope-like nesting, so after run the most recent token would be removed. It's a bit different from other thread-static things we have: node code ensures that the proper phase and logger are set/copied before the computations and that the previous state is restored afterwards on the current thread, so subsequent computations start in this state. I've tried to do the same with the list of the cancellation tokens, but it didn't work as good, as I wasn't able to ensure we're clearing the state on the correct thread with the possible thread switching and in parallel node computations.

In this PR I'm making it more similar to the logger/phase logic: we only have one token and set/copy it before/after the computation. This requires setting the token somewhere in the outer code, so, just like the logger/phase, the token is set inside the background compiler, e.g. when starting ParseAndCheckFileInProject and other features.

With this implementation I couldn't yet reproduce access to a module reader without cancellation token being set. The FCS cancellation tests also work as expected. Let's see what does the CI say.

[auduchinok]

I'm still thinking about a possibly better approach, but the current changes look promising to me for now to keep testing it. I'm curious about trying to make it lower-level and more automatic for the features code again in future. This could be especially good if node/cancellable/async expressions are unified in service/checker, so only one CE builder would need to know about this token and all the features code would just use it automatically.

[vzarytovskii]

I'm still thinking about a possibly better approach, but the current changes look promising to me for now to keep testing it. I'm curious about trying to make it lower-level and more automatic for the features code again in future. This could be especially good if node/cancellable/async expressions are unified in service/checker, so only one CE builder would need to know about this token and all the features code would just use it automatically.

Yeah, I'll probably bite the bullet and try to unify them under state-machine based one. In December, when things are a bit more quiet.

[0101]

Is there a chance we will "leak" a token to some thread and cancel some computation we didn't intend? Do we need to ensure that we call UsingToken at some point before every cancellable computation? I suppose that makes sense, just wondering what if we forget - could we get some rare unexpected cancellations?

[auduchinok]

Is there a chance we will "leak" a token to some thread and cancel some computation we didn't intend?

I can't say for sure whether the token may "leak" to another thread, but it's working in the same way as the logger/phase now. The guard from using a wrong token is setting the token in the top level code, and then node will copy it on the new thread before running the computation.

Do we need to ensure that we call UsingToken at some point before every cancellable computation? I suppose that makes sense, just wondering what if we forget - could we get some rare unexpected cancellations?

Yes, with this change we need to call UsingToken to wrap the FCS computations. And, yes, if we forget to do it somewhere, it may be unexpectedly cancelled. That's why I'd prefer the more automatic approach where it's hidden in the computation implementation, but it's currently more difficult to do as different places use different computation types.

[0101]

I would definitely prefer the automated way also. But wondering what would be a proper way to do it. Even if all computations used the same builder, how would it know when it's time to set the top-level token? We probably don't want to create a new one every time. And we can't check if "it's already there", because that could mean someone else put it on this particular thread. So we'd probably need to ensure that we restore the thread's state whenever we switch out of it? If we could do that that would also take care of any issues with this current approach (if we forget to set it somewhere).

[auduchinok]

But wondering what would be a proper way to do it.

Setting it before the computation and returning the old state after, similar to the previous approach in Cancellable.run, sounds good.

So we'd probably need to ensure that we restore the thread's state whenever we switch out of it? If we could do that that would also take care of any issues with this current approach (if we forget to set it somewhere).

It would be great indeed, but that's what I haven't find a way to do yet, unfortunately. Let's have a call if you want to chat about it more and/or have some ideas to try. 🙂

[auduchinok]

I propose we merge it like this for now, this seems good to try to actually use it. I'll be happy to revisit it when/if there're changes in the builders or around the transparent compiler.

[psfinaki]

Thanks Eugene. Let's get this in :)