No more segfaults: a new approach for greening existing locks

[itamarst]

1. Works better on older Python 3.
2. Doesn't cause segfaults!!!!
3. Somewhat worse in terms of finding RLocks than what we had before, but in practice unlikely to be an issue.
4. Adds a warning about unupgraded RLocks.
5. Improved test coverage.
6. As of latest iteration, only a few milliseconds slower than previous version.

Fixes #864

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (a94a3cf) 54% compared to head (239aa6a) 54%.

Files	Patch %	Lines
eventlet/patcher.py	83%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##           master   #866   +/-   ##
=====================================
  Coverage      54%    54%           
=====================================
  Files          88     88           
  Lines        9744   9741    -3     
  Branches     1814   1811    -3     
=====================================
+ Hits         5297   5302    +5     
+ Misses       4081   4076    -5     
+ Partials      366    363    -3     

Flag	Coverage Δ
ipv6	22% <6%> (+<1%)	⬆️
py310epolls	53% <83%> (+<1%)	⬆️
py310poll	53% <83%> (+<1%)	⬆️
py310selects	52% <83%> (+<1%)	⬆️
py311epolls	53% <83%> (+<1%)	⬆️
py312epolls	50% <83%> (+<1%)	⬆️
py37epolls	50% <83%> (+<1%)	⬆️
py38epolls	53% <83%> (+<1%)	⬆️
py38openssl	51% <83%> (+<1%)	⬆️
py38poll	53% <83%> (+<1%)	⬆️
py38selects	52% <83%> (+<1%)	⬆️
py39dnspython1	51% <83%> (+<1%)	⬆️
py39epolls	53% <83%> (+<1%)	⬆️
py39poll	53% <83%> (+<1%)	⬆️
py39selects	52% <83%> (+<1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[itamarst]

Will fix test failures later today.

[tipabu]

Should we worry about keys?

for k, v in list(container.items()):
    add_or_traverse(k)
    add_or_traverse(v)

[itamarst]

Main reason to support dict values is that we previously were updating things like function locals. Not doing that anymore, but potentially someone has some class that uses mapping dictionaries for attributes. We could support keys but that's changing the way you look things up, which seems iffier, and... beyond a certain point we can't handle every case. So we could, but a better answer is "monkey patch early", or better yet "don't use eventlet".

[itamarst]

Another example of something we could upgrade is a set that contains RLocks. And something we maybe could do but would be a lot harder is tuples.

[tipabu]

if k is old:
    if v is old:
        container[new] = new
    else:
        container[new] = container[k]
    del container[k]
elif v is old:
    ...

?
A diff like

diff --git a/tests/isolated/patcher_existing_locks_preexisting.py b/tests/isolated/patcher_existing_locks_preexisting.py
index e3306ee..04be763 100644
--- a/tests/isolated/patcher_existing_locks_preexisting.py
+++ b/tests/isolated/patcher_existing_locks_preexisting.py
@@ -16,7 +16,7 @@ class NS:
     class NS2:
         lock = threading.RLock()
 
-    dict = {1: 2, 12: threading.RLock()}
+    dict = {1: 2, 12: threading.RLock(), threading.RLock(): 3, NS2.lock: NS2.lock}
     list = [0, threading.RLock()]
 
 
@@ -35,6 +35,10 @@ if __name__ == '__main__':
     ensure_upgraded(NS.lock)
     ensure_upgraded(NS.NS2.lock)
     ensure_upgraded(NS.dict[12])
+    for k in NS.dict:
+        if not isinstance(k, int):
+            ensure_upgraded(k)
+    ensure_upgraded(NS.dict[NS.NS2.lock])
     ensure_upgraded(NS.list[1])
     if sys.version_info[:2] > (3, 9):
         ensure_upgraded(unittest.mock.NonCallableMock._lock)

should demonstrate the existing trouble.

[tipabu]

I'm confused about how this passes with these prints...

Oh, e51f72a started allowing multiple lines as long as the last one was pass... why'd we do that?

Oh. Hmm...

Program .../tests/isolated/patcher_fork_after_monkey_patch.py output:
---
.../tests/isolated/patcher_fork_after_monkey_patch.py:38: DeprecationWarning: This process (pid=166324) is multi-threaded, use of fork() may lead to deadlocks in the child.
  if os.fork() == 0:
pass

---

[itamarst]

Yeah, I made those test framework changes because of noise like that.

[tipabu]

Strange: running this directly, I trip the RuntimeError here, but it passes when run through tests/patcher_test.py...

[itamarst]

Weird, it passes for me if I run it directly.

[4383]

Both launches types works for me.

[4383]

Why it is fine to use gc.get_objects for logging purposes? I'm not sure to understand that reason and I think I won't be the only one.

Using gc.get_objects won't work with Python release lower than 3.10 and we still support Python 3.7.

That would lead to at least to remaining_rlocks = 0 or in worst case to a runtime error, no?
Then, logs won't show the following error.

So, either this comment should be updated to explain why using gc.get_objects is not a problem, or the code should be modified to do not call this method, or I misunderstood something. Thoughts?

[itamarst]

It's just useful information. On older Python you will get less useful information, but it doesn't hurt you in any way. I updated the docstring to try to reflect this.

[4383]

Thanks for the details

[4383]

LGTM

[4383]

Both launches types works for me.