Will need to think about `on.exit` about coro HOT 21 CLOSED

We could use a bool that'd be set to TRUE when the generator is exiting normally (with a return or a pause). Then if it's FALSE we'd know on.exit is run from an unexpected longjump. If that's the case we could mark the generator as broken and make it throw an error if reentered.

I also think that a generator that has returned shouldn't be reentered and should fail if called again.

With this we should be able to allow three kind of exit expressions. We'd provide custom operators for them and users should probably not be allowed to use on.exit(). We'd have:

• error exits
• return exits
• pause exits

from coro.

hmm on second thought we should probably follow standard R semantics and only have combined return+error exits. The pause exits still seem like a good idea.

from coro.

As for the three kinds of exits, I think we could just have:

• stop() for errors
• return() for normal exit, and
• yield()/await() for continuation

from coro.

I was talking about the kinds of situations in which exit expressions are called. We probably need to distinguish between pauses and other types of exits. This does mean the user might hold on to resources until the generator has completely exited...

We probably need to add the non-pause exits to the GC finaliser of the generator in case they were not run.

from coro.

I think we shouldn't special-case stop() or any other causes of exit.

If the function exits for any cause other than a pause, we should run the exit expressions, mark the generator as terminated, and prevent any reentry.

from coro.

It might be useful to have is_terminated() and reset() operators for generators.

from coro.

Holding on to resources until the generator has completely exited is (almost?) always bad practice IMO. You cannot know what other code is running asynchronously. I am not sure if we want to support that. AFAIK the idiom is to get the lock (or set the context) for every call, and release the lock (or reset the context) at every exit (every kind of exit).

As for is_terminated(), how about allowing a finalizer on the generator? I guess a reset() would just create another instance, so it is not crucial, although might be handy sometimes.

from coro.

That's where the pause exits will be useful. But I think it makes sense to have the general exit as well.

from coro.

TBH I would just forbid on.exit completely.

from coro.

I think it could be useful because generators will also be used for other things than concurrency. But the more immediate problem is pausing exits so we have time to think about it.

from coro.

I think it could be useful because generators will also be used for other things than concurrency.

Sure, it is just hard to make sure that on.exit expressions run, and they run early enough. E.g. on error, if we run them in the finalizer, that might be to late, you might get into a deadlock before the finalizer gets a chance to run.

Also, even if you create a generator for a non-concurrency reason, it might just run concurrently with something, hence there is a chance to deadlock.

I would argue for a withr::with_* kind of idiom, i.e. we could provide some helpers to run each generator call in some well defined context, and we would also make sure that the context is restored and locks are released at each exit from the generator.

from coro.

On error we'd run them in the exit expression and we'd disable the finaliser. The finaliser would only be used if the exit expression was not run.

As for deadlocks they are just bugs. I don't think we should prevent users from writing deadlocks just as they should be allowed to write infinite loops ;)

from coro.

The finaliser would only be used if the exit expression was not run.

Still, when the finalizer runs, that might be too late.

As for deadlocks they are just bugs. I don't think we should prevent users from writing deadlocks just as they should be allowed to write infinite loops ;)

The main problem with allowing users holding on to resources until the very end, is that the behavior of your code depends on what other code it is used with. Even without on.exit, people can do this, of course, but on.exit even encourages it. I would rather help the idiomatic method, with some helpers. E.g. promises have some helpers I think, because Shiny needs them, to set the graphics device, etc.

The infinite loop is a bad example, because most of the time whether or not you write an infinite loop is up to you. These deadlocks happen if some user uses your code together with somebody else's code.

from coro.

I think we just need a terminate() operator to allow more control over resource freeing when the generator is not properly terminated.

from coro.

That said I totally agree that we need scoped resource helpers that open and close a resource on entry and pause.

from coro.

In the end I agree that we shouldn't bother with the general exit because it will be too much work to get right, might not be that useful, and you're right that it might encourage bad code.

from coro.

OK, great. I think at some point we could try to detect it, but initially we could just document, that you should not use on.exit() in generators and async functions.

from coro.

on.exit() can be overscoped in the gen env. We'll miss base::on.exit() calls but they're not very common.

from coro.

Relevant regarding general exit clean up: golang has defer in goroutines which people use for cleaning up resources.

from coro.

The goroutines deferred expressions are only guaranteed to execute if the goroutines are synchronised e.g. with a waitGroup. That makes a lot of sense: this circumscribes the area where resources are not closed and it makes deadlocks local bugs (local to the group of synchronised concurrent processes).

So I think a general on_exit probably does not make sense for generators but perhaps makes sense for async-await processes (it makes sense to call them processes in the CSP sense, they are just not communicating through channels). And we do need a terminate() operator to make sure the scheduler properly cleans up all processes when R aborts.

from coro.

Yeah. The async package already has cancellation, and it is not hard to implement. Basically you just cancel all other "processes" that are waiting on the cancelled one. Cancellation is just a condition so you can tryCatch it.

from coro.