os.environ raises RuntimeError: dictionary changed size during iteration

[osantana]

BPO	30441
Nosy	@mdickinson, @pitrou, @vadmium, @serhiy-storchaka, @osantana, @JelleZijlstra
PRs	bpo-30441: Fix bug when modifying os.environ while iterating over it #2409 [3.6] bpo-30441: Fix bug when modifying os.environ while iterating over it (GH-2409). #2556 [3.5] bpo-30441: Fix bug when modifying os.environ while iterating over it (GH-2409). #2557
Files	fix_os_environ_iteration_issue.diff: patch with fix and test fix_os_environ_iter_issue_low_level_thread_lock.diff: (use this) use _thread.allocate_lock() as lock provider fix_os_environ_iter_issue_threading_lock.diff fix_os_environ_iter_issue_low_level_thread_lock_2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-07-04.05:19:33.068>
created_at = <Date 2017-05-23.14:35:50.699>
labels = ['3.7', 'type-bug', 'library']
title = 'os.environ raises RuntimeError: dictionary changed size during iteration'
updated_at = <Date 2017-07-05.03:46:48.399>
user = 'https://github.com/osantana'

bugs.python.org fields:

activity = <Date 2017-07-05.03:46:48.399>
actor = 'osantana'
assignee = 'none'
closed = True
closed_date = <Date 2017-07-04.05:19:33.068>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-05-23.14:35:50.699>
creator = 'osantana'
dependencies = []
files = ['46889', '46896', '46897', '46902']
hgrepos = []
issue_num = 30441
keywords = ['patch']
message_count = 27.0
messages = ['294250', '294257', '294261', '294352', '294354', '294398', '294406', '294407', '294444', '294445', '294452', '294453', '294456', '294457', '294459', '294460', '294467', '294468', '294498', '296122', '296847', '296904', '297486', '297625', '297626', '297627', '297695']
nosy_count = 7.0
nosy_names = ['mark.dickinson', 'pitrou', 'martin.panter', 'serhiy.storchaka', 'osantana', 'JelleZijlstra', 'Danilo Shiga']
pr_nums = ['2409', '2556', '2557']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30441'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[osantana]

We're facing ocasional RuntimeError exceptions in a multithreaded application when one of the threads creates new entries to the environment (os.environ).

I'm not so sure if the attached patch fixes this issue the right way, so, feel free to propose another approach.

Traceback Sample of the issue in our production environment:

RuntimeError: dictionary changed size during iteration
File "python3.5/runpy.py", line 170, in _run_module_as_main
"__main__", mod_spec)
File "python3.5/runpy.py", line 85, in _run_code
exec(code, run_globals)

[ internal code ]

File "newrelic/api/background_task.py", line 103, in wrapper
return wrapped(*args, **kwargs)
File "python3.5/contextlib.py", line 30, in inner
return func(*args, **kwds)
File "python3.5/contextlib.py", line 77, in __exit__
self.gen.throw(type, value, traceback)
File "raven/base.py", line 851, in make_decorator
yield
File "python3.5/contextlib.py", line 30, in inner
return func(*args, **kwds)

[ internal code ]

File "retry/api.py", line 74, in retry_decorator
logger)
File "retry/api.py", line 33, in __retry_internal
return f()

[ internal code ]

File "requests/sessions.py", line 531, in get
return self.request('GET', url, **kwargs)
File "requests/sessions.py", line 509, in request
prep.url, proxies, stream, verify, cert
File "requests/sessions.py", line 686, in merge_environment_settings
env_proxies = get_environ_proxies(url, no_proxy=no_proxy)
File "requests/utils.py", line 696, in get_environ_proxies
return getproxies()
File "urllib/request.py", line 2393, in getproxies_environment
for name, value in os.environ.items():
File "_collections_abc.py", line 676, in __iter__
for key in self._mapping:
File "python3.5/os.py", line 702, in __iter__
for key in self._data:

[JelleZijlstra]

Even with the patch, I don't think it's safe to modify os.environ while it's being accessed concurrently in another thread. The other thread's modification could arrive while the dict() call in your patch is running (in CPython the GIL might protect you, but that's an implementation detail).

I think the real solution is that your application uses a lock or some other concurrency mechanism to protect access to os.environ.

[osantana]

I agree with Jelle about the fix's implementation. But I disagree of suggestion to implement lock at the application side.

I disagree because some external libraries may want to access os.environ in a concurrent fashion without lock acquiring. That's the case reported in my previous comment: urllib/requests.py (https://github.com/python/cpython/blob/master/Lib/urllib/request.py#L2468) iterates over os.environ with no lock control.

How could I handle this issue in this case? Would it be a good idea implement the lock mechanism in current os._Environ (https://github.com/python/cpython/blob/master/Lib/os.py#L666) mapping class?

[vadmium]

Previous report: bpo-25641. At least in Posix, the “putenv” function is not required to be thread safe.

[osantana]

There are exceptions being raised in many applications (as reported here and in http://bugs.python.org/issue25641) and there are three paths to follow:

1. We handle this exception somewhere;
2. We avoid raising it;
3. Just leave it. I don't care about this exception.

If we chose to handle this exception we need to decide where we'll do it. At os.py module? At urllib/request.py? At all http client libraries (eg. python-requests)? At our applications?

If we chose to avoid this exception we, probably, need to implement some kind of lock/mutex in os.environ.

I can implement any of these options. You just need to decide which one is best.

If we chose the third option, we can just close this issue.

[pitrou]

The environ class could have its own lock and then the __iter__ method could be rewritten as follows:

    def __iter__(self):
        with self._lock:
            keys = list(self._data)
        for key in keys:
            yield self.decodekey(key)

[osantana]

This patch implements a lock protection (as suggested by Antoine) using _thread.allocate_lock() module (instead of threading.Lock()) due to the fact that CPython interpreter already imports _thread module during its bootstrap process.

[osantana]

This is an alternative implementation using threading.Lock(). The main issue with this implementation relies on the fact that we've to import the threading module during CPython interpreter loading (currently it's not imported by default).

This is consequence of the fact that CPython interpreter uses os module when bootstrapping.

[mdickinson]

@osantana: you'd need to be using the lock at least in __setitem__ and __delitem__ as well, else there's nothing preventing modifications to the dictionary during the list call. It may make sense to protect all accesses (read or write) to _data, though you'd need to either use a re-entrant lock in that case or be very careful about not acquiring the lock twice.

[pitrou]

I'm fine with a low-level lock, though as Mark says you could use _thread._RLock().

[serhiy-storchaka]

Isn't list(dict) atomic? At least in CPython.

I suspect that protecting *all* accesses to _data with a lock will hit the performance and invalidate the reason of using _data for caching.

[pitrou]

Isn't list(dict) atomic? At least in CPython.

I doubt it. There's no special code for list(dict), it uses regular iteration which could probably hit the GC and then release the GIL.

I suspect that protecting *all* accesses to _data with a lock will hit the performance and invalidate the reason of using _data for caching.

I'm not convinced os.environ is performance-critical. Perhaps getenv() is really expensive on some systems?