-
Notifications
You must be signed in to change notification settings - Fork 141
Prevent Runtime use over forks
#1208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…prevent_default to allow users to more easily enforce that a default runtime is never automatically created
…nstrate that you can call prevent_default and then set_default to allow future calls to default. add tests for set_default
cretz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, non-blocking concern about if we need the entire new RuntimeRef class to add a global boolean
temporalio/runtime.py
Outdated
| ) -> None: | ||
| self._default_runtime: Runtime | None = None | ||
| self._prevent_default = False | ||
| self._default_created = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this field needed if _default_runtime's None-ness can be used as basically the same thing? And therefore if you have default runtime and prevent default, do they deserve an entirely new class/encapsulation as opposed to just adding a single _prevent_default global alongside the existing global?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original thinking for _default_created was to differentiate between the default being lazily created and one being set via set_default. I can see that not being worth tracking and just raising in prevent_default if an existing _default_runtime exists. I think I'll make that change because it similarly makes sense to avoid calling prevent_default after set_default. Even though that would be harmless, it is a weird thing to do.
The primary purpose of the encapsulation was just to write tests in a way that plays nicely with existing fixtures. Definitely happy to consider alternatives here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, if we have to extract out to a new class for test only, ok. It makes for a bit uglier stack trace for users, but not a big deal.
temporalio/runtime.py
Outdated
| _default_runtime = runtime | ||
| global _runtime_ref | ||
| _runtime_ref.set_default(runtime, error_if_already_set=error_if_already_set) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return |
…lag in favor of using the optional nature of _default_runtime in _RuntimeRef
Raise an assertion error when a Runtime is used by client/worker creation/usage.
What was changed
RuntimeErrorif the runtime was created by a different process._RuntimeRefprivate class to encapsulate behavior around managing calls toRuntime.default_default_runtimewith an instance of the new_RuntimeRefand use it in calls toRuntime.default,Runtime.prevent_defaultandRuntime.set_default.Why?
Startup time in large codebases can be challenge to manage. One strategy to manage startup time is to load modules once and then fork the process into many instances so the child process don't need to repeat the startup cost. These changes help users employing that strategy to manage Temporal usage across forks in a safe manner.
Checklist
Closes [Feature Request] Better prevention of accidental
Runtimeuse over forks #1201How was this tested:
Client.connectafterRuntime.defaultwas called pre-forkRuntime.defaultwas called pre-fork_RuntimeRefclass that the lifecycle of default runtime.