search

temporalio / sdk-python

Temporal Python SDK
MIT License
473 stars 77 forks source link

During eviction, set is_replaying and raise special exception #524

Closed cretz closed 6 months ago

cretz commented 6 months ago

What was changed

Checklist

  1. Closes #523
Sushisource commented 6 months ago

Something I don't quite understand about the statement in the issue:

anyone in a finally or except Exception can catch exceptions occurring during eviction and do side-effecting things like logging

Why would the user code be woken up at all during eviction? We shouldn't, ideally, even run the tasks containing the user code at all. Then we wouldn't need to do anything special here.

cretz commented 6 months ago

Why would the user code be woken up at all during eviction? We shouldn't, ideally, even run the tasks containing the user code at all. Then we wouldn't need to do anything special here.

Because that's the only way you can collect paused coroutines in many languages (including Java, Go, and Python). The whole issue with #499 is that we were letting Python GC wake these things up in whatever thread. Now we wake them up in our controlled environment. This is the same as Java where you could technically catch a io.temporal.internal.sync.DestroyWorkflowThreadError.

cretz commented 6 months ago

I was curious about if Python really must raise that, and it seems like it maaaaybe doesn't, it happens as a consequence of close() getting called on generators: https://docs.python.org/3/reference/expressions.html#generator.close

There is no way to prevent it. We've tried everything. The proper way, like Go and Java, is to force coroutines to complete themselves and ignore side effects.

I wonder if we could override close for the generators which constitute user's workflow code and tasks started therein to simply... not do that? Maybe something worth considering, but I can imagine it might have a bunch of unexpected consequences too.

https://github.com/python/cpython/blob/v3.12.3/Lib/_collections_abc.py#L176 occurs and there's nothing we can find to prevent this. And yes, I think they require it to ensure garbage collection of the coroutine so unsure what consequences it would have if we even could prevent it.