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:

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.

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?

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

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.

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)

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.

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.

@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.

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

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.

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?

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

While there are no special code for list(dict), regular iteration over exact dict don't hit the GC.

While there are no special code for list(dict), regular iteration over exact dict don't hit the GC.

Doesn't it? Creating a dict iterator creates a GC-tracked object.

But it is destroyed only after iterating.

Every time you create a GC-tracked object, you can invoke the GC. See _PyObject_GC_Alloc.

Invoking GC before iterating or after iterating doesn't break the atomicity of iterating.

Perhaps. But this discussion shows the matter is complicated and we shouldn't rely on fragile assumptions about implementation details. So we need a lock.

New version of patch:

1. Use RLock instead of Lock (Mark & Antoine recomendation)
2. Add lock acquire at __setitem__ and __delitem__ as following (Mark & Antoine recomendation)
3. Add lock protection in __repr__ implementation.

I've some questions pending:

1. Is it a good idea to wrap self._data access with lock in __getitem__? (I suspect it's not)
2. Is it a good idea to lock protect self.copy() and self.setdefault() (I suspect it's not because both uses __iter__ that is already protected)

See also bpo-24484. The solution depends on the assumption that list(dict) is atomic.

Osvaldo, could you please create a pull request on GitHub based on your first patch? But use list() instead of dict().

Pull Request bpo-2409 (#2409) opened.

New changeset 8a8d285 by Serhiy Storchaka (Osvaldo Santana Neto) in branch 'master':
bpo-30441: Fix bug when modifying os.environ while iterating over it (bpo-2409)
8a8d285

New changeset bebd2cf by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30441: Fix bug when modifying os.environ while iterating over it (GH-2409). (bpo-2556)
bebd2cf

New changeset 1a3bc55 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30441: Fix bug when modifying os.environ while iterating over it (GH-2409). (bpo-2557)
1a3bc55

Thank you for your contribution Osvaldo.

Hi Serhiy Storchaka,

I need to thank you for your valuable guidance.