bpo-30441: Fix bug when modifying os.environ while iterating over it#2409
bpo-30441: Fix bug when modifying os.environ while iterating over it#2409serhiy-storchaka merged 7 commits intopython:masterfrom osantana:fix-os-environ-iteration
Conversation
osantana
changed the title
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Please add also a NEWS entry in Misc/NEWS.d/next/Library and your name in Misc/ACKS.
Lib/os.py
Outdated
|
|
||
| def __iter__(self): | ||
| for key in self._data: | ||
| data = list(self._data) |
There was a problem hiding this comment.
Either inline data or rename it to keys. Add an explaining comment about atomicity of list() from dict.
Lib/test/test_os.py
Outdated
| self.assertTrue(cm.exception.__suppress_context__) | ||
|
|
||
| def test_iter_error_when_os_environ_changes(self): | ||
| def _iter_environ_change(): |
There was a problem hiding this comment.
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()).
Lib/test/test_os.py
Outdated
|
|
||
| # add a new key in os.environ mapping | ||
| new_key = "__{}".format(key) | ||
| os.environ[new_key] = value |
There was a problem hiding this comment.
Don't forget to remove the added key.
No. Creating a tuple from a dict is not atomic now. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Add an entry in Misc/NEWS.d.
Misc/ACKS
Outdated
| Gennadiy Zlobin | ||
| Doug Zongker | ||
| Peter Åstrand | ||
| Osvaldo Santana Neto |
There was a problem hiding this comment.
Names are ordered in alphabetical order. You name presumably should be under the letter N.
There was a problem hiding this comment.
Are you sure? I didn't see an alphabetical order, so, I assumed I should put my name at the end of file...
There was a problem hiding this comment.
Oh! I see now!... It's alphabetical order of surname (not the first name)
Lib/test/test_os.py
Outdated
| next(iterator) # force iteration over modified mapping | ||
| self.assertEqual(os.environ[new_key], "test_environ_iteration") | ||
|
|
||
| del os.environ[new_key] |
There was a problem hiding this comment.
The new key should be removed even if the test is failed, otherwise it could affect other tests. Use try...finally.
Lib/test/test_os.py
Outdated
|
|
||
| def test_iter_error_when_changing_os_environ_values(self): | ||
| iter_environ_values = iter(os.environ.values()) | ||
| self._test_environ_iteration(iter_environ_values) |
There was a problem hiding this comment.
Why repeat "iter environ values" so much times? Why not inline the iter_environ_values variable?
serhiy-storchaka
added
needs backport to 3.5
type-bug
…er it (pythonGH-2409). (cherry picked from commit 8a8d285)
|
GH-2556 is a backport of this pull request to the 3.6 branch. |
…er it (pythonGH-2409). (cherry picked from commit 8a8d285)
|
GH-2557 is a backport of this pull request to the 3.5 branch. |
4 participants
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?