Skip to content

Fix unhandling of possible recursion error on ExceptionRecord.__deepcopy__ - #870

Merged
xpconanfan merged 1 commit into
google:masterfrom
uael:uael/exception-record-deepcopy-recursion-error
Feb 15, 2023
Merged

Fix unhandling of possible recursion error on ExceptionRecord.__deepcopy__#870
xpconanfan merged 1 commit into
google:masterfrom
uael:uael/exception-record-deepcopy-recursion-error

Conversation

@uael

@uael uael commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

While using mobly and aio gRPC, the following very long trace pop on gRPC aio error. Although this might not be Mobly related, since deepcopy can trigger a RecursionError I think it make sense to handle it. This change fix that.

Trace (cut for readability):

AioRpcError
[ExampleTest] 02-06 15:47:34.895 ERROR Exception happened when executing on_fail for test.
Traceback (most recent call last):
  File "venv/lib/python3.10/site-packages/mobly/base_test.py", line 640, in _exec_procedure_func
    func(copy.deepcopy(tr_record))
  File "/usr/lib/python3.10/copy.py", line 172, in deepcopy
    y = _reconstruct(x, memo, *rv)
  File "/usr/lib/python3.10/copy.py", line 271, in _reconstruct
    state = deepcopy(state, memo)
  File "/usr/lib/python3.10/copy.py", line 146, in deepcopy
    y = copier(x, memo)
  File "/usr/lib/python3.10/copy.py", line 231, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
  File "/usr/lib/python3.10/copy.py", line 153, in deepcopy
    y = copier(memo)
  File "venv/lib/python3.10/site-packages/mobly/records.py", line 280, in __deepcopy__
    exception = copy.deepcopy(self.exception)
  File "/usr/lib/python3.10/copy.py", line 172, in deepcopy
    y = _reconstruct(x, memo, *rv)
  File "/usr/lib/python3.10/copy.py", line 265, in _reconstruct
    y = func(*args)
  File "/usr/lib/python3.10/copy.py", line 264, in <genexpr>
    args = (deepcopy(arg, memo) for arg in args)
  File "/usr/lib/python3.10/copy.py", line 172, in deepcopy
    y = _reconstruct(x, memo, *rv)
  File "/usr/lib/python3.10/copy.py", line 265, in _reconstruct
    y = func(*args)
  File "/usr/lib/python3.10/copy.py", line 264, in <genexpr>
    args = (deepcopy(arg, memo) for arg in args)
  ...
RecursionError: maximum recursion depth exceeded  

This change is Reviewable

@xpconanfan xpconanfan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @uael)


mobly/records.py line 280 at r1 (raw file):

    try:
      exception = copy.deepcopy(self.exception)
    except (TypeError, RecursionError):

can you share the data that cause this to be raised?
I'm not sure if the same logic is correct for handling RecursionError, so let's take a look at the root cause to be sure

Also, we should have unit tests to cover this case :)

@xpconanfan xpconanfan added the bug label Feb 6, 2023
@xpconanfan xpconanfan added this to the Mobly Release 1.12.2 milestone Feb 6, 2023
@uael
uael force-pushed the uael/exception-record-deepcopy-recursion-error branch from 5c82f43 to da35578 Compare February 6, 2023 19:25
@uael

uael commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

After some more investigation here is what's going on.

The AioRpcError from grpcio has what I see has a bogus initialization by passing self to the base constructor which actually create a self recursive exception:

python/grpcio/grpc/aio/_call.py:89

    def __init__(self, ...):
        super().__init__(self)

The RecursionError would have been raised here if __str__ was not overridden:

mobly/records.py:248

    else:
      self._set_details(e)

It's raising in the deepcopy instead:

mobly/records.py:279

    try:
      exception = copy.deepcopy(self.exception)
    except TypeError:

Even if the recursion is clearly a bug in grpcio (I'm gonna open PR for it), since nothing prevent an error to be recursive, I still think this change to be relevant.

I'm not sure if the same logic is correct for handling RecursionError

I think using the exception as is fine too since it cannot be copied.

I also just added a specific unit test.
Here is the pytest trace of the added test without the fix:

======================================================================================================================================== FAILURES ========================================================================================================================================
__________________________________________________________________________________________________________________ RecordsTest.test_recursive_exception_record_deepcopy __________________________________________________________________________________________________________________

self = <tests.mobly.records_test.RecordsTest testMethod=test_recursive_exception_record_deepcopy>

    def test_recursive_exception_record_deepcopy(self):
      """Makes sure ExceptionRecord wrapper handles deep copy properly in case of recursive exception."""
      try:
        raise RecordTestRecursiveError()
      except RecordTestRecursiveError as e:
        er = records.ExceptionRecord(e)
>     new_er = copy.deepcopy(er)

tests/mobly/records_test.py:450: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.10/copy.py:153: in deepcopy
    y = copier(memo)
mobly/records.py:279: in __deepcopy__
    exception = copy.deepcopy(self.exception)
/usr/lib/python3.10/copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
/usr/lib/python3.10/copy.py:265: in _reconstruct
    y = func(*args)
/usr/lib/python3.10/copy.py:264: in <genexpr>
    args = (deepcopy(arg, memo) for arg in args)
/usr/lib/python3.10/copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
E   RecursionError: maximum recursion depth exceeded while calling a Python object
!!! Recursion detected (same locals & position)

uael added a commit to uael/grpc that referenced this pull request Feb 6, 2023
xpconanfan
xpconanfan previously approved these changes Feb 8, 2023
@uael
uael force-pushed the uael/exception-record-deepcopy-recursion-error branch from da35578 to ab738ff Compare February 9, 2023 01:05
gnossen pushed a commit to grpc/grpc that referenced this pull request Feb 10, 2023
@uael
uael requested a review from xpconanfan February 15, 2023 00:47
Comment thread tests/mobly/records_test.py Outdated
xpconanfan
xpconanfan previously approved these changes Feb 15, 2023
@uael
uael force-pushed the uael/exception-record-deepcopy-recursion-error branch from ab738ff to 2d3bd3d Compare February 15, 2023 01:38
@uael
uael requested a review from xpconanfan February 15, 2023 01:39

@uael uael left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @uael and @xpconanfan)


mobly/records.py line 280 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

can you share the data that cause this to be raised?
I'm not sure if the same logic is correct for handling RecursionError, so let's take a look at the root cause to be sure

Also, we should have unit tests to cover this case :)

Done.


tests/mobly/records_test.py line 444 at r2 (raw file):

Previously, xpconanfan (Ang Li) wrote…

pls make sure the line stays within line length limit of 80 chars

Done.

@xpconanfan
xpconanfan merged commit caf37d0 into google:master Feb 15, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
wanlin31 pushed a commit to grpc/grpc that referenced this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants