Skip to content
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

bpo-30441: Fix bug when modifying os.environ while iterating over it #2409

Merged
merged 7 commits into from Jul 1, 2017

Conversation

Projects
None yet
4 participants
@osantana
Copy link
Contributor

commented Jun 26, 2017

Opening this pull request at Serhiy Storchaka's request to correct Bug #30441.

Question: Would not it be better to use tuple() instead of list() at Lib/os.py:700?

@osantana osantana changed the title Fix bug when modifying os.environ while iterating over it (Bug #30441) bpo-30441: Fix bug when modifying os.environ while iterating over it Jun 26, 2017

@serhiy-storchaka
Copy link
Member

left a comment

Please add also a NEWS entry in Misc/NEWS.d/next/Library and your name in Misc/ACKS.

Lib/os.py Outdated
@@ -697,7 +697,8 @@ def __delitem__(self, key):
raise KeyError(key) from None

def __iter__(self):
for key in self._data:
data = list(self._data)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 27, 2017

Member

Either inline data or rename it to keys. Add an explaining comment about atomicity of list() from dict.

@@ -835,6 +835,21 @@ def test_key_type(self):
self.assertIs(cm.exception.args[0], missing)
self.assertTrue(cm.exception.__suppress_context__)

def test_iter_error_when_os_environ_changes(self):
def _iter_environ_change():

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 27, 2017

Member

You can just use iter(os.environ.items()).

It may be worth to test also other iterators: iter(os.environ) and iter(os.environ.values()).


# add a new key in os.environ mapping
new_key = "__{}".format(key)
os.environ[new_key] = value

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 27, 2017

Member

Don't forget to remove the added key.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Question: Would not it be better to use tuple() instead of list() at Lib/os.py:700?

No. Creating a tuple from a dict is not atomic now.

@serhiy-storchaka
Copy link
Member

left a comment

Add an entry in Misc/NEWS.d.

Misc/ACKS Outdated
@@ -1760,3 +1760,4 @@ Jelle Zijlstra
Gennadiy Zlobin
Doug Zongker
Peter Åstrand
Osvaldo Santana Neto

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 30, 2017

Member

Names are ordered in alphabetical order. You name presumably should be under the letter N.

This comment has been minimized.

Copy link
@osantana

osantana Jul 1, 2017

Author Contributor

Are you sure? I didn't see an alphabetical order, so, I assumed I should put my name at the end of file...

This comment has been minimized.

Copy link
@osantana

osantana Jul 1, 2017

Author Contributor

Oh! I see now!... It's alphabetical order of surname (not the first name)

next(iterator) # force iteration over modified mapping
self.assertEqual(os.environ[new_key], "test_environ_iteration")

del os.environ[new_key]

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 30, 2017

Member

The new key should be removed even if the test is failed, otherwise it could affect other tests. Use try...finally.


def test_iter_error_when_changing_os_environ_values(self):
iter_environ_values = iter(os.environ.values())
self._test_environ_iteration(iter_environ_values)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 30, 2017

Member

Why repeat "iter environ values" so much times? Why not inline the iter_environ_values variable?

@serhiy-storchaka serhiy-storchaka merged commit 8a8d285 into python:master Jul 1, 2017

2 checks passed

bedevere/issue-number Issue number 30441 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jul 4, 2017

@bedevere-bot

This comment has been minimized.

Copy link

commented Jul 4, 2017

GH-2556 is a backport of this pull request to the 3.6 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jul 4, 2017

@bedevere-bot

This comment has been minimized.

Copy link

commented Jul 4, 2017

GH-2557 is a backport of this pull request to the 3.5 branch.

serhiy-storchaka added a commit that referenced this pull request Jul 4, 2017

serhiy-storchaka added a commit that referenced this pull request Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.