make iterating over sys.modules threadsafe

[mmohrhard]

Despite creating a copy through list(sys.modules.items()) there
is a possible race condition if another thread is adding to sys.modules

  File "x/lib/python3.7/pickle.py", line 774, in save_tuple
    save(element)
  File "x/lib/python3.7/pickle.py", line 549, in save
    self.save_reduce(obj=obj, *rv)
  File "x/lib/python3.7/pickle.py", line 637, in save_reduce
    save(func)
  File "x/lib/python3.7/pickle.py", line 518, in save
    self.save_global(obj)
  File "x/lib/python3.7/site-packages/cloudpickle/cloudpickle.py", line 876, in save_global
    elif not _is_global(obj, name=name):
  File "x/lib/python3.7/site-packages/cloudpickle/cloudpickle.py", line 174, in _is_global
    module_name = _whichmodule(obj, name)
  File "x/lib/python3.7/site-packages/cloudpickle/cloudpickle.py", line 156, in _whichmodule
    for module_name, module in list(sys.modules.items()):
RuntimeError: dictionary changed size during iteration

[mmohrhard]

I think the CI failures are unrelated.

[ogrisel]

Thank you very much @mmohrhard? Do you think you could write a non-regression test for this?

Also please add an entry in the CHANGES.md file.

[codecov]

Codecov Report

Merging #322 into master will decrease coverage by 34.63%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #322       +/-   ##
===========================================
- Coverage   92.96%   58.33%   -34.64%     
===========================================
  Files           2        2               
  Lines         853      852        -1     
  Branches      178      175        -3     
===========================================
- Hits          793      497      -296     
- Misses         37      321      +284     
- Partials       23       34       +11

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	79.52% <100%> (-12.5%)	⬇️
cloudpickle/cloudpickle_fast.py	0% <0%> (-95.6%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cf9ec4...fe35eb3. Read the comment docs.

[mmohrhard]

@ogrisel I have added the Changes.md part but am unable to generate a test case that fails before the fix. We have only hit the bug now 3 times out of a hundred production runs and only after updating one of our dependencies. Based on trying to create a MCVE it seems it is quite tough to generate a test that hits this condition reliably.

[ogrisel]

@mmohrhard thanks for your feedback.

However there is something that I do not understand: both list(l) and l.copy are builtin functions and none of them mentions explicitly that the copy is thread-safe if l is being concurrently mutated in another thread.

So I have not sure that using .copy is actually the correct fix. @pitrou do you have a suggestion?

[pitrou]

list.copy is pretty much thread-safe AFAICT. As for dict.copy though, it's a different can of worms...

[mmohrhard]

list.copy is pretty much thread-safe AFAICT.

The patch is replacing creating a list from a dict items view (in the end using PyDictIterItem_Type) to copying the dict and then creating the items view. From reading PyDict_Copy looks completely thread-safe whereas apparently list_extend in Object/listobject.c is not.

[mmohrhard]

I should also note that I have not yet completely figured out how list_extend is able to drop the GIL in this case. We have now hit the same bug at least once in cpython's pickle.py which contains the same piece of code. I'm still trying to debug the code to understand why the list(sys.modules.items()) code is not thread-safe.

[pitrou]

The GIL can be dropped at various places where pure Python code is executed, for example when running the GC (which will trigger execution of destructors etc.). Basically, C code that is guaranted not to release the GIL is the exception rather than the rule.

[ogrisel]

list.copy is pretty much thread-safe AFAICT. As for dict.copy though, it's a different can of worms...

Sorry I meant dict.copy indeed.

So what is the solution to iterate over sys.modules in a thread safe way?

for _ in range(100):
    try:
        sys_modules_copy = sys.modules.copy()
        break
    except RuntimeError:
        pass  # try again
else:
    raise RuntimeError("Failed to iterate over sys.modules")

for module_name, module in sys_modules_copy.items():
    ...

That looks really ugly to me...

[ogrisel]

Let's merge this because empirically it seems to reduce the likelihood to get a race condition. Still if someone has a better suggestion feel free to comment or open a new PR.

[mmohrhard]

Just for documentation, Serhiy Storchaka explains in https://bugs.python.org/issue40327 the underlying problem and mentions why the sys.modules.copy().items() pattern is slightly better. Cpython is now also replacing the list(sys.modules.items()) pattern.