Skip to content

Conversation

@sjperkins
Copy link
Member

In the existing implementation, tracebacks from exceptions aren't properly garbage collected.
The strategy in this PR avoids this.
Tested in #6281 when enabling previously disabled check_instance tests.

  • Tests added / passed
  • Passes pre-commit run --all-files

Comment on lines +778 to +783
i = self.unroll_stack

while traceback.tb_next and i > 0:
traceback = traceback.tb_next
i -= 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
i = self.unroll_stack
while traceback.tb_next and i > 0:
traceback = traceback.tb_next
i -= 1
for _ in range(self.unroll_stack):
if not traceback.tb_next:
break
traceback = traceback.tb_next

@crusaderky crusaderky changed the title Modfy log_errors contextmanager to avoid traceback leaks Modify log_errors context manager to avoid traceback leaks May 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

Unit Test Results

       16 files  ±0         16 suites  ±0   7h 34m 33s ⏱️ - 11m 3s
  2 767 tests ±0    2 688 ✔️ +2       78 💤  - 2  1 ±0 
22 098 runs  ±0  21 070 ✔️  - 6  1 020 💤  - 1  8 +7 

For more details on these failures, see this check.

Results for commit 50814e0. ± Comparison against base commit 8411c2d.

@crusaderky
Copy link
Collaborator

ERROR    test_utils:utils.py:788 err4
Traceback (most recent call last):
  File "/home/runner/work/distributed/distributed/distributed/utils.py", line 765, in wrapper
    return func(*args, **kwargs)
  File "/home/runner/work/distributed/distributed/distributed/tests/test_utils.py", line 862, in inner
    raise CustomError("err4")
test_utils.test_log_errors.<locals>.CustomError: err4

I'm a bit worried that outer is nowhere to be found in the stack trace - and also that the output is now on stderr.
The use case that err4 replicates is a very common one in our codebase.
Could you find out what's happening?
Please let me know if you get stuck.

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.

2 participants