-
-
Notifications
You must be signed in to change notification settings - Fork 16
add .unwrap_and_destroy() method to remove reference to value/error when unwrapping outcome #49
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?
Changes from 4 commits
b5f58f2
bdaedf3
2b70089
5fd1f61
b4bb54c
13d7d13
73c5f72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Add ``unwrap_and_destroy`` method to remove references to | ||
| the wrapped exception or value to prevent issues where | ||
| values not being garbage collected when they are no longer | ||
| needed, or worse problems with exceptions leaving a | ||
| reference cycle. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -138,6 +138,23 @@ def unwrap(self) -> ValueT: | |||||
| x = fn(*args) | ||||||
| x = outcome.capture(fn, *args).unwrap() | ||||||
|
|
||||||
| Note: this leaves a reference to the contained value or exception | ||||||
| alive which may result in values not being garbage collected or | ||||||
| exceptions leaving a reference cycle. If this is an issue it's | ||||||
| recommended to call the ``unwrap_and_destroy()`` method | ||||||
|
|
||||||
| """ | ||||||
|
|
||||||
| @abc.abstractmethod | ||||||
| def unwrap_and_destroy(self) -> ValueT: | ||||||
| """Return or raise the contained value or exception, remove the | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| reference to the contained value or exception. | ||||||
|
|
||||||
| These two lines of code are equivalent:: | ||||||
|
|
||||||
| x = fn(*args) | ||||||
| x = outcome.capture(fn, *args).unwrap_and_destroy() | ||||||
|
|
||||||
| """ | ||||||
|
|
||||||
| @abc.abstractmethod | ||||||
|
|
@@ -174,12 +191,21 @@ class Value(Outcome[ValueT], Generic[ValueT]): | |||||
| """The contained value.""" | ||||||
|
|
||||||
| def __repr__(self) -> str: | ||||||
| return f'Value({self.value!r})' | ||||||
| try: | ||||||
| return f'Value({self.value!r})' | ||||||
| except AttributeError: | ||||||
| return 'Value(<AlreadyDestroyed>)' | ||||||
|
|
||||||
| def unwrap(self) -> ValueT: | ||||||
| self._set_unwrapped() | ||||||
| return self.value | ||||||
|
|
||||||
| def unwrap_and_destroy(self) -> ValueT: | ||||||
| self._set_unwrapped() | ||||||
| v = self.value | ||||||
| object.__delattr__(self, "value") | ||||||
| return v | ||||||
|
|
||||||
| def send(self, gen: Generator[ResultT, ValueT, object]) -> ResultT: | ||||||
| self._set_unwrapped() | ||||||
| return gen.send(self.value) | ||||||
|
|
@@ -202,7 +228,10 @@ class Error(Outcome[NoReturn]): | |||||
| """The contained exception object.""" | ||||||
|
|
||||||
| def __repr__(self) -> str: | ||||||
| return f'Error({self.error!r})' | ||||||
| try: | ||||||
| return f'Error({self.error!r})' | ||||||
| except AttributeError: | ||||||
| return 'Error(<AlreadyDestroyed>)' | ||||||
|
|
||||||
| def unwrap(self) -> NoReturn: | ||||||
| self._set_unwrapped() | ||||||
|
|
@@ -226,6 +255,29 @@ def unwrap(self) -> NoReturn: | |||||
| # __traceback__ from indirectly referencing 'captured_error'. | ||||||
| del captured_error, self | ||||||
|
|
||||||
| def unwrap_and_destroy(self) -> NoReturn: | ||||||
| self._set_unwrapped() | ||||||
| # Tracebacks show the 'raise' line below out of context, so let's give | ||||||
| # this variable a name that makes sense out of context. | ||||||
| captured_error = self.error | ||||||
| object.__delattr__(self, "error") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really necessary to do here? Or could we move this into the |
||||||
| try: | ||||||
| raise captured_error | ||||||
| finally: | ||||||
| # We want to avoid creating a reference cycle here. Python does | ||||||
| # collect cycles just fine, so it wouldn't be the end of the world | ||||||
| # if we did create a cycle, but the cyclic garbage collector adds | ||||||
| # latency to Python programs, and the more cycles you create, the | ||||||
| # more often it runs, so it's nicer to avoid creating them in the | ||||||
| # first place. For more details see: | ||||||
| # | ||||||
| # https://github.com/python-trio/trio/issues/1770 | ||||||
| # | ||||||
| # In particuar, by deleting this local variables from the 'unwrap' | ||||||
| # methods frame, we avoid the 'captured_error' object's | ||||||
| # __traceback__ from indirectly referencing 'captured_error'. | ||||||
| del captured_error, self | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably isn't necessary anymore:
Suggested change
and I remember hearing that CPython does some magic with names pointing to the exception being thrown, so this line might be completely unnecessary? But maybe I'm misremembering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Only for >>> exc = ...
>>> "exc" in locals()
True
>>> try:
... raise RuntimeError
... except BaseException as exc:
... "exc" in locals()
True
>>> "exc" in locals()
False |
||||||
|
|
||||||
| def send(self, gen: Generator[ResultT, NoReturn, object]) -> ResultT: | ||||||
| self._set_unwrapped() | ||||||
| return gen.throw(self.error) | ||||||
|
|
||||||
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.
I think we should focus on the root cause of any issues, like maybe (wording very much a first draft):
(and also maybe make this note block a warning?)