Skip to content

fix: RuntimeError: dictionary changed size during iteration raised within extract_locals #298

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

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

shenek
Copy link
Contributor

@shenek shenek commented Mar 20, 2019

  • affects only python3 because items() in python3 doesn't return list
  • note that this situation is actually a simplified example how environ is treated in Bottle framework

@untitaker
Copy link
Member

That's messed up. Can you link me to the code in bottle that changes the environ during repr?

@@ -30,3 +51,15 @@ def test_basic(sentry_init, crashing_app, capture_events):
"query_string": "",
"url": "http://localhost/",
}


def test_env_modifing_(sentry_init, crashing_env_modifing_app, capture_events):
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix this name to something that doesn't have an underscore at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sorry I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There supposed to be _app

@shenek shenek force-pushed the self-modifing-structure branch from d267e6e to ae8e794 Compare March 20, 2019 09:10
…raised in extract_locals phase

* affects only python3 because items() in python3 doesn't return list
* note that this situation is actually a simplified example how environ is treated in Bottle framework
@shenek shenek force-pushed the self-modifing-structure branch from ae8e794 to 30f1ec6 Compare March 20, 2019 09:12
@shenek
Copy link
Contributor Author

shenek commented Mar 20, 2019

The code itself in bottle is not very 'visible'. And it took a quite amount of time to debug. But I can at least show you a part of my trace.

So lets start with repr: https://github.com/bottlepy/bottle/blob/0.12.16/bottle.py#L1386
Here you call self.url which calls self.urlparts.
see https://github.com/bottlepy/bottle/blob/0.12.16/bottle.py#L1252
The code itself still looks ok (no modifications to environ are done).
But there is this DictProperty decorator.
It creates a new element bottle.request.urlparts in environ when it is called for the first time.
see https://github.com/bottlepy/bottle/blob/0.12.16/bottle.py#L166

@untitaker
Copy link
Member

untitaker commented Mar 20, 2019

Amazing. I'll include this in 0.7.7. Thanks!

@untitaker untitaker merged commit 99900d1 into getsentry:master Mar 20, 2019
@shenek
Copy link
Contributor Author

shenek commented Mar 20, 2019

You're welcomed.

@untitaker
Copy link
Member

FYI, with #368 this will soon spit out a placeholder instead of the value. Using list(dict.items()) causes severe performance issues for some of our customers. Let me know if you have a better suggestion.

@shenek
Copy link
Contributor Author

shenek commented May 17, 2019

FYI, with #368 this will soon spit out a placeholder instead of the value. Using list(dict.items()) causes severe performance issues for some of our customers. Let me know if you have a better suggestion.

I'm not sure, but perhaps something like this could help:

diff --git a/sentry_sdk/_compat.py b/sentry_sdk/_compat.py
index 871995e..5dbf158 100644
--- a/sentry_sdk/_compat.py
+++ b/sentry_sdk/_compat.py
@@ -1,14 +1,28 @@
 import sys
+import copy
 
 if False:
     from typing import Optional
     from typing import Tuple
     from typing import Any
     from typing import Type
+    from typing import Callable
 
 
 PY2 = sys.version_info[0] == 2
 
+
+def safe_iteritems_wrapper(unsafe_iter):
+    # type: (Callable[[dict], None]) -> Callable[[dict], None]
+    def wrapped(x):
+        try:
+            return unsafe_iter(x)
+        except RuntimeError:
+            return copy.copy(x).items()
+
+    return wrapped
+
+
 if PY2:
     import urlparse  # noqa
 
@@ -52,6 +66,9 @@ else:
         raise value
 
 
+iteritems = safe_iteritems_wrapper(iteritems)
+
+
 def with_metaclass(meta, *bases):
     class metaclass(type):
         def __new__(cls, name, this_bases, d):

@untitaker
Copy link
Member

Do you think an easy fix for this could be that we just serialize the event breadth-first?

@shenek
Copy link
Contributor Author

shenek commented May 17, 2019

Do you think an easy fix for this could be that we just serialize the event breadth-first?

I would doubt that. The problem is that you are modifying the dict which are you currently iterating.

              A
             /\  \
current -> A1 A2 (B1) <- inserted element
          / \
        AA1 AA2

I guess that it would only have failed sooner. I'll try to run some test for that later.

@untitaker
Copy link
Member

Nevermind, list(dict.items()) is basically what I meant with breadth-first. But I figured out a way to restore the old behavior without impacting perf, all good.

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