From f48922ce09b419ca9d7576da87e8ca294ff30ba7 Mon Sep 17 00:00:00 2001 From: Terseus Date: Mon, 31 Oct 2022 23:14:10 +0100 Subject: [PATCH] Fix not calling all the exit stack when record_on_exception is False The initial technique to implement `record_on_exception=False` was to not emptying the generator returned by `CassetteContextDecorator._patch_generator` when an exception happens to skip the `cassette._save` call, however this had the side effect of not emptying the `ExitStack` created inside the generator which contains the `_patch.__exit__` calls to remove the patches. This was innocuous in CPython, which uses a reference counting garbage collector so the `ExitStack` was immediately collected after losing scope and therefore its `__exit__` method executed. Pypy, on the other hand, uses a generational garbage collector so its objects may survive more time, enough for the `ExitStack` not called until much later, which may cause the patches to live more than expected when `record_on_exception=False`. This was found because the test `test_nesting_context_managers_by_checking_references_of_http_connection` was failing because it was executed after `test_dont_record_on_exception`. Now the cassette instance is saved inside the `CassetteContextDecorator` instance to have better control on where to save the cassette, and moved the `cassette._save` call from the `_patch_generator` method to the `__exit__` method to be free to empty the generator and remove the patches always. --- vcr/cassette.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/vcr/cassette.py b/vcr/cassette.py index 36d4e9e..5822afa 100644 --- a/vcr/cassette.py +++ b/vcr/cassette.py @@ -59,6 +59,7 @@ class CassetteContextDecorator: self.cls = cls self._args_getter = args_getter self.__finish = None + self.__cassette = None def _patch_generator(self, cassette): with contextlib.ExitStack() as exit_stack: @@ -68,9 +69,6 @@ class CassetteContextDecorator: log.debug(log_format.format(action="Entering", path=cassette._path)) yield cassette log.debug(log_format.format(action="Exiting", path=cassette._path)) - # TODO(@IvanMalison): Hmmm. it kind of feels like this should be - # somewhere else. - cassette._save() def __enter__(self): # This assertion is here to prevent the dangerous behavior @@ -88,14 +86,23 @@ class CassetteContextDecorator: if other_kwargs.get("path_transformer"): transformer = other_kwargs["path_transformer"] cassette_kwargs["path"] = transformer(cassette_kwargs["path"]) - self.__finish = self._patch_generator(self.cls.load(**cassette_kwargs)) + self.__cassette = self.cls.load(**cassette_kwargs) + self.__finish = self._patch_generator(self.__cassette) return next(self.__finish) def __exit__(self, *exc_info): exception_was_raised = any(exc_info) record_on_exception = self._args_getter().get("record_on_exception", True) if record_on_exception or not exception_was_raised: - next(self.__finish, None) + self.__cassette._save() + self.__cassette = None + # Fellow programmer, don't remove this `next`, if `self.__finish` is + # not consumed the unpatcher functions accumulated in the `exit_stack` + # object created in `_patch_generator` will not be called until + # `exit_stack` is not garbage collected. + # This works in CPython but not in Pypy, where the unpatchers will not + # be called until much later. + next(self.__finish, None) self.__finish = None @wrapt.decorator