bpo-38091: Import deadlock detection causes deadlock

[arigo]

https://bugs.python.org/issue38091

Automerge-Triggered-By: @brettcannon

[pitrou]

The fix here is more of a hack. A proper fix could be something like the below. At least it fixes the reproducer posted by @rlamy . Is there a real-world reproducer you can test it on?

diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
index 8de0e9ee79..7b2bfefdec 100644
--- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -71,9 +71,20 @@ class _ModuleLock:
             lock = _blocking_on.get(tid)
             if lock is None:
                 return False
-            tid = lock.owner
-            if tid == me:
+            owner_tid = lock.owner
+            if owner_tid == tid:
+                # Lock owner is temporarily blocking on a lock it
+                # already owns.  This means it's not actually blocking.
+                # (this can happen at the beginning of acquire() below
+                #  -- see bpo 38091)
+                return False
+            if owner_tid == me:
+                # Lock owner is blocking on a lock we own, but we're blocking
+                # on the lock it owns => deadlock.
                 return True
+            # Move along the chain and find out whether the owner is blocking
+            # on another lock.
+            tid = owner_tid
 
     def acquire(self):
         """

[arigo]

Maybe a safer way to think about it would be to ask ourselves when has_deadlock() is really supposed to return True. I think it means to return True if the chain _blocking_on[_blocking_on[...[tid]...]] eventually reaches the value me, and False in all other cases. You are fixing one such case by detecting when the chain enters a fixpoint, i.e. a loop of length 1. I'm not convinced that longer loops are impossible; maybe we should also detect them and return False in these cases. Something like that (untested code):

     def has_deadlock(self):
         # Deadlock avoidance for concurrent circular imports.
         me = _thread.get_ident()
         tid = self.owner
+        seen = set()
         while True:
             lock = _blocking_on.get(tid)
             if lock is None:
                 return False
             tid = lock.owner
             if tid == me:
                 return True
+            if tid in seen:
+                # bpo 38091: the chain of tid's we encounter here
+                # eventually leads to a fixpoint or a cycle, but
+                # does not reach 'me'.  This means we would not
+                # actually deadlock.  This can happen if other
+                # threads are at the beginning of acquire() below.
+                return False    
+            seen.add(tid)

[pitrou]

@arigo Yes, that fix would probably work. Perhaps @rlamy can check :-)

[rlamy]

I think that this should fix the issue. It caused some intermittent test failures in PyPy, so I guess we should just merge the change there and monitor the CI results.

[pitrou]

@rlamy Did you test this fix on PyPy?

[brettcannon]

Marking as rejected until we here back from PyPy that the proposed fix in the PR discussion worked for them.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[arigo]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

[arigo]

I have updated the PR to include the test from https://bugs.python.org/issue38091 , which fails (deadlocks) without the fix and passes with the fix.

[brettcannon]

importlib.h wasn't regenerated and included in this PR. I believe that can be done with make regen-all (or something like that).

[bedevere-bot]

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

[codecov]

Codecov Report

Merging #17518 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #17518     +/-   ##
=========================================
  Coverage   82.11%   82.12%             
=========================================
  Files        1955     1954      -1     
  Lines      588906   584027   -4879     
  Branches    44429    44458     +29     
=========================================
- Hits       483572   479607   -3965     
+ Misses      95679    94777    -902     
+ Partials     9655     9643     -12     

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	30.00% <0.00%> (-65.00%)	⬇️
Lib/distutils/command/bdist_rpm.py	7.63% <0.00%> (-56.88%)	⬇️
Lib/test/test_urllib2net.py	76.92% <0.00%> (-13.85%)	⬇️
Lib/test/test_smtpnet.py	78.57% <0.00%> (-7.15%)	⬇️
Lib/ftplib.py	63.85% <0.00%> (-6.06%)	⬇️
Lib/test/test_ftplib.py	87.11% <0.00%> (-4.72%)	⬇️
Tools/scripts/db2pickle.py	17.82% <0.00%> (-3.97%)	⬇️
Tools/scripts/pickle2db.py	16.98% <0.00%> (-3.78%)	⬇️
Lib/test/test_socket.py	71.94% <0.00%> (-3.77%)	⬇️
Lib/distutils/tests/test_bdist_msi.py	56.25% <0.00%> (-3.75%)	⬇️
... and 359 more

---

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 bec4186...1b2dbe4. Read the comment docs.

[miss-islington]

@arigo: Status check is done, and it's a failure ❌ .

[miss-islington]

@arigo: Status check is done, and it's a success ✅ .

[miss-islington]

Thanks @arigo for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry, @arigo, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6daa37fd42c5d5300172728e8b4de74fe0b319fc 3.8

[miss-islington]

Sorry @arigo, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 6daa37fd42c5d5300172728e8b4de74fe0b319fc 3.7

[brettcannon]

@arigo any interest in manually backporting this to 3.8 and 3.7? (Chances are it's importlib.h.)

[arigo]

My git-fu is very limited (in general and on github). Do you expect two other pull requests for the 3.7 and 3.8 branches?

[brettcannon]

@arigo yep, but don't worry about it too much. I can try to get around to it eventually myself.