Skip to content

During eviction, set is_replaying and raise special exception#524

Merged
cretz merged 3 commits into
temporalio:mainfrom
cretz:eviction-as-replaying
May 15, 2024
Merged

During eviction, set is_replaying and raise special exception#524
cretz merged 3 commits into
temporalio:mainfrom
cretz:eviction-as-replaying

Conversation

@cretz
Copy link
Copy Markdown
Contributor

@cretz cretz commented May 9, 2024

What was changed

  • Set workflow.unsafe.is_replaying() to True during eviction
  • Raise a special BaseException-based exception during eviction instead of RuntimeError which has an Exception base

Checklist

  1. Closes [Bug] Make sure is_replaying is True upon eviction, and only throw certain exception types out on eviction #523

@cretz cretz requested a review from a team as a code owner May 9, 2024 21:02
@Sushisource
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

cretz commented May 13, 2024

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.

Copy link
Copy Markdown
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, now I remember that. 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

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.

Anyway, this works for me but it might be worth checking out

@cretz
Copy link
Copy Markdown
Contributor Author

cretz commented May 15, 2024

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.

@cretz cretz merged commit a52f25d into temporalio:main May 15, 2024
@cretz cretz deleted the eviction-as-replaying branch May 15, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Make sure is_replaying is True upon eviction, and only throw certain exception types out on eviction

2 participants