Skip to content

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

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.

Sign up for GitHub

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
serhiy-storchaka merged 7 commits into from
Jul 1, 2017
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
375f977
Fix bug when modifying os.environ while iterating over it (Issue #30441)
osantana Jun 26, 2017
cac586f
Rename data->keys and explain about list() atomicity
osantana Jun 29, 2017
6bbfe8c
Improve tests and add more use-cases
osantana Jun 29, 2017
2190931
Add entry in NEWS.d/Library and author name in Misc/ACKS
osantana Jun 29, 2017
3b15ce0
Fix typo in test names and improve var names
osantana Jun 29, 2017
a061788
Refactor tests to improve readability
osantana Jul 1, 2017
4c3f4da
Fix surname alphabetical order in Misc/ACKS
osantana Jul 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lib/os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,9 @@ def __delitem__(self, key):
raise KeyError(key) from None

def __iter__(self):
for key in self._data:
# list() from dict object is an atomic operation
keys = list(self._data)
for key in keys:
yield self.decodekey(key)

def __len__(self):
Expand Down
25 changes: 25 additions & 0 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,31 @@ def test_key_type(self):
self.assertIs(cm.exception.args[0], missing)
self.assertTrue(cm.exception.__suppress_context__)

def _test_environ_iteration(self, iterator):
new_key = "__new_key__"

next(iterator) # start iteration over os.environ.items

# add a new key in os.environ mapping
os.environ[new_key] = "test_environ_iteration"

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

del os.environ[new_key]
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Jun 30, 2017

Choose a reason for hiding this comment

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

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(self):
iter_environ = iter(os.environ)
self._test_environ_iteration(iter_environ)

def test_iter_error_when_changing_os_environ_items(self):
iter_environ_items = iter(os.environ.items())
self._test_environ_iteration(iter_environ_items)

def test_iter_error_when_changing_os_environ_values(self):
iter_environ_values = iter(os.environ.values())
self._test_environ_iteration(iter_environ_values)
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Jun 30, 2017

Choose a reason for hiding this comment

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

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



class WalkTests(unittest.TestCase):
"""Tests for os.walk()."""
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1760,3 +1760,4 @@ Jelle Zijlstra
Gennadiy Zlobin
Doug Zongker
Peter Åstrand
Osvaldo Santana Neto
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Jun 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@osantana osantana Jul 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@osantana osantana Jul 1, 2017

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2017-06-29-14-25-14.bpo-30441.3Wh9kc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug when modifying os.environ while iterating over it