-
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
Open
VegetarianOrc
wants to merge
12
commits into
main
Choose a base branch
from
prevent-fork
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
5a9268c
Raise an assertion error when a Runtime is used by client/worker crea…
VegetarianOrc 41213ef
Add _RuntimeRef to encapsulate default runtime creation. Add Runtime.…
VegetarianOrc 59156b2
remove blank line to fix linter error
VegetarianOrc 5f93b8e
fix use of Self since it's not avaiable in typing until 3.11
VegetarianOrc 811aa64
remove references to ForkContext to avoid exploding in Windows
VegetarianOrc 2461cdd
switch type of fixture to Iterator instead of Generator
VegetarianOrc ba9a8a8
run formatter
VegetarianOrc 9301c51
except the correct error type to prevent breakage on windows
VegetarianOrc c350ed5
Update tests to match error info. Update prevent_default test to demo…
VegetarianOrc db1ede0
fix typo in docstring
VegetarianOrc 0f9bc70
remove empty return in Runtime.set_default. Remove _default_created f…
VegetarianOrc f81fd55
Merge branch 'main' into prevent-fork
VegetarianOrc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -24,33 +24,83 @@ | |||
| import temporalio.bridge.runtime | ||||
| import temporalio.common | ||||
|
|
||||
| _default_runtime: Optional[Runtime] = None | ||||
|
|
||||
| class _RuntimeRef: | ||||
| def __init__( | ||||
| self, | ||||
| ) -> None: | ||||
| self._default_runtime: Runtime | None = None | ||||
| self._prevent_default = False | ||||
| self._default_created = False | ||||
|
|
||||
| def default(self) -> Runtime: | ||||
| if not self._default_runtime: | ||||
| if self._prevent_default: | ||||
| raise RuntimeError( | ||||
| "Cannot create default Runtime after Runtime.prevent_default has been called" | ||||
| ) | ||||
| self._default_runtime = Runtime(telemetry=TelemetryConfig()) | ||||
| self._default_created = True | ||||
| return self._default_runtime | ||||
|
|
||||
| def prevent_default(self): | ||||
| if self._default_created: | ||||
| raise RuntimeError( | ||||
| "Runtime.prevent_default called after default runtime has been created" | ||||
| ) | ||||
| self._prevent_default = True | ||||
|
|
||||
| def set_default( | ||||
| self, runtime: Runtime, *, error_if_already_set: bool = True | ||||
| ) -> None: | ||||
| if self._default_runtime and error_if_already_set: | ||||
| raise RuntimeError("Runtime default already set") | ||||
|
|
||||
| self._default_runtime = runtime | ||||
|
|
||||
|
|
||||
| _runtime_ref: _RuntimeRef = _RuntimeRef() | ||||
|
|
||||
|
|
||||
| class Runtime: | ||||
| """Runtime for Temporal Python SDK. | ||||
|
|
||||
| Users are encouraged to use :py:meth:`default`. It can be set with | ||||
| Most users are encouraged to use :py:meth:`default`. It can be set with | ||||
| :py:meth:`set_default`. Every time a new runtime is created, a new internal | ||||
| thread pool is created. | ||||
|
|
||||
| Runtimes do not work across forks. | ||||
| Runtimes do not work across forks. Advanced users should consider using | ||||
| :py:meth:`prevent_default` and `:py:meth`set_default` to ensure each | ||||
| fork creates it's own runtime. | ||||
|
|
||||
| """ | ||||
|
|
||||
| @classmethod | ||||
| def default(cls) -> Runtime: | ||||
| """Get the default runtime, creating if not already created. | ||||
| """Get the default runtime, creating if not already created. If :py:meth:`prevent_default` | ||||
| is called before this method it will raise a RuntimeError instead of creating a default | ||||
| runtime. | ||||
|
|
||||
| If the default runtime needs to be different, it should be done with | ||||
| :py:meth:`set_default` before this is called or ever used. | ||||
|
|
||||
| Returns: | ||||
| The default runtime. | ||||
| """ | ||||
| global _default_runtime | ||||
| if not _default_runtime: | ||||
| _default_runtime = cls(telemetry=TelemetryConfig()) | ||||
| return _default_runtime | ||||
| global _runtime_ref | ||||
| return _runtime_ref.default() | ||||
|
|
||||
| @classmethod | ||||
| def prevent_default(cls): | ||||
| """Prevent :py:meth:`default` from lazily creating a :py:class:`Runtime`. | ||||
|
|
||||
| Raises a RuntimeError if a default :py:class:`Runtime` has already been created. | ||||
|
|
||||
| Explicitly setting a default runtime with :py:meth:`set_default` bypasses this setting and | ||||
| future calls to :py:meth:`default` will return the provided runtime. | ||||
| """ | ||||
| global _runtime_ref | ||||
| _runtime_ref.prevent_default() | ||||
|
|
||||
| @staticmethod | ||||
| def set_default(runtime: Runtime, *, error_if_already_set: bool = True) -> None: | ||||
|
|
@@ -65,10 +115,9 @@ def set_default(runtime: Runtime, *, error_if_already_set: bool = True) -> None: | |||
| error_if_already_set: If True and default is already set, this will | ||||
| raise a RuntimeError. | ||||
| """ | ||||
| global _default_runtime | ||||
| if _default_runtime and error_if_already_set: | ||||
| raise RuntimeError("Runtime default already set") | ||||
| _default_runtime = runtime | ||||
| global _runtime_ref | ||||
| _runtime_ref.set_default(runtime, error_if_already_set=error_if_already_set) | ||||
| return | ||||
|
||||
| return |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import multiprocessing | ||
| import multiprocessing.context | ||
| import sys | ||
| from dataclasses import dataclass | ||
| from typing import Any | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| @dataclass | ||
| class _ForkTestResult: | ||
| status: str | ||
| err_name: str | None | ||
| err_msg: str | None | ||
|
|
||
| def __eq__(self, value: object) -> bool: | ||
| if not isinstance(value, _ForkTestResult): | ||
| return False | ||
|
|
||
| valid_err_msg = False | ||
|
|
||
| if self.err_msg and value.err_msg: | ||
| valid_err_msg = ( | ||
| self.err_msg in value.err_msg or value.err_msg in self.err_msg | ||
| ) | ||
|
|
||
| return ( | ||
| value.status == self.status | ||
| and value.err_name == value.err_name | ||
| and valid_err_msg | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def assertion_error(message: str) -> _ForkTestResult: | ||
| return _ForkTestResult( | ||
| status="error", err_name="AssertionError", err_msg=message | ||
| ) | ||
|
|
||
|
|
||
| class _TestFork: | ||
| _expected: _ForkTestResult | ||
|
|
||
| async def coro(self) -> Any: | ||
| raise NotImplementedError() | ||
|
|
||
| def entry(self): | ||
| event_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(event_loop) | ||
| try: | ||
| event_loop.run_until_complete(self.coro()) | ||
| payload = _ForkTestResult(status="ok", err_name=None, err_msg=None) | ||
| except BaseException as err: | ||
| payload = _ForkTestResult( | ||
| status="error", err_name=err.__class__.__name__, err_msg=str(err) | ||
| ) | ||
|
|
||
| self._child_conn.send(payload) | ||
| self._child_conn.close() | ||
|
|
||
| def run(self, mp_fork_context: multiprocessing.context.BaseContext | None): | ||
| process_factory = getattr(mp_fork_context, "Process", None) | ||
|
|
||
| if not mp_fork_context or not process_factory: | ||
| pytest.skip("fork context not available") | ||
|
|
||
| self._parent_conn, self._child_conn = mp_fork_context.Pipe(duplex=False) | ||
| # start fork | ||
| child_process = process_factory(target=self.entry, args=(), daemon=False) | ||
| child_process.start() | ||
| # close parent's handle on child_conn | ||
| self._child_conn.close() | ||
|
|
||
| # get run info from pipe | ||
| payload = self._parent_conn.recv() | ||
| self._parent_conn.close() | ||
|
|
||
| assert payload == self._expected |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'sNone-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_defaultglobal 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_createdwas to differentiate between the default being lazily created and one being set viaset_default. I can see that not being worth tracking and just raising inprevent_defaultif an existing_default_runtimeexists. I think I'll make that change because it similarly makes sense to avoid callingprevent_defaultafterset_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.
Uh oh!
There was an error while loading. Please reload this page.
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.