Add a test for #651 and really fix #651 this time (another manifestation of...

Add a test for #651 and really fix #651 this time (another manifestation of the same underlying issue as #652)

Makefile

@@ -40,7 +40,7 @@ doc:
 	cd doc && PYTHONPATH=.. make html
 
 whitespace:
-	! find . -not -path "./.tox/*" -not -path "*/__pycache__/*" -not -path "*.so" -not -path "*.pyc" -not -path "./.git/*" -not -path "./build/*" -not -path "./libev/*" -not -path "./gevent/libev/*" -not -path "./gevent.egg-info/*" -not -path "./dist/*" -not -path "./.DS_Store" -not -path "./c-ares/*" -not -path "./gevent/gevent.*.[ch]" -not -path "./gevent/core.pyx" -not -path "./doc/_build/*" -not -path "./doc/mytheme/static/*" -type f | xargs egrep -l " $$"
+	! find . -not -path "./.eggs/*" -not -path "./greentest/htmlcov/*" -not -path "./greentest/.coverage.*" -not -path "./.tox/*" -not -path "*/__pycache__/*" -not -path "*.so" -not -path "*.pyc" -not -path "./.git/*" -not -path "./build/*" -not -path "./libev/*" -not -path "./gevent/libev/*" -not -path "./gevent.egg-info/*" -not -path "./dist/*" -not -path "./.DS_Store" -not -path "./c-ares/*" -not -path "./gevent/gevent.*.[ch]" -not -path "./gevent/core.pyx" -not -path "./doc/_build/*" -not -path "./doc/mytheme/static/*" -type f | xargs egrep -l " $$"
 
 pep8:
 	${PYTHON} `which pep8` .

gevent/_tblib.py

@@ -228,7 +228,25 @@ def install():
 # Added by gevent
 
 # We have to defer the initilization, and especially the import of platform,
-# until runtime.
+# until runtime. If we're monkey patched, we need to be sure to use
+# the original __import__ to avoid switching through the hub due to
+# import locks on Python 2. See also builtins.py for details.
+
+
+def _unlocked_imports(f):
+    def g(a):
+        gb = None
+        if 'gevent.builtins' in sys.modules:
+            gb = sys.modules['gevent.builtins']
+            gb._unlock_imports()
+        try:
+            return f(a)
+        finally:
+            if gb is not None:
+                gb._lock_imports()
+    g.__name__ = f.__name__
+    g.__module__ = f.__module__
+    return g
 
 
 def _import_dump_load():
@@ -271,11 +289,16 @@ def _init():
     install()
 
 
+@_unlocked_imports
 def dump_traceback(tb):
+    # Both _init and dump/load have to be unlocked, because
+    # copy_reg and pickle can do imports to resolve class names; those
+    # class names are in this module and greenlet safe though
     _init()
     return dumps(tb)
 
 
+@_unlocked_imports
 def load_traceback(s):
     _init()
     return loads(s)

gevent/builtins.py

@@ -29,6 +29,8 @@ _import = builtins.__import__
 # module lock is used such that this fix is not necessary.
 _g_import_lock = gevent.lock.RLock()
 
+__lock_imports = True
+
 
 def __import__(*args, **kwargs):
     """
@@ -45,7 +47,10 @@ def __import__(*args, **kwargs):
         args = args[1:]
     # TODO: It would be nice not to have to acquire the locks
     # if the module is already imported (in sys.modules), but the interpretation
-    # of the arguments is somewhat complex
+    # of the arguments is somewhat complex.
+    if not __lock_imports:
+        return _import(*args, **kwargs)
+
     imp.acquire_lock()
     try:
         _g_import_lock.acquire()
@@ -57,6 +62,27 @@ def __import__(*args, **kwargs):
         imp.release_lock()
     return result
 
+
+def _unlock_imports():
+    """
+    Internal function, called when gevent needs to perform imports
+    lazily, but does not know the state of the system. It may be impossible
+    to take the import lock because there are no other running greenlets, for
+    example. This causes a monkey-patched __import__ to avoid taking any locks.
+    until the corresponding call to lock_imports. This should only be done for limited
+    amounts of time and when the set of imports is statically known to be "safe".
+    """
+    global __lock_imports
+    # This could easily become a list that we push/pop from or an integer
+    # we increment if we need to do this recursively, but we shouldn't get
+    # that complex.
+    __lock_imports = False
+
+
+def _lock_imports():
+    global __lock_imports
+    __lock_imports = True
+
 if sys.version_info[:2] >= (3, 3):
     __implements__ = []
 else:

gevent/greenlet.py

@@ -446,12 +446,13 @@ class Greenlet(greenlet):
         switch = getcurrent().switch
         self.rawlink(switch)
         try:
-            t = Timeout.start_new(timeout)
+            t = Timeout.start_new(timeout) if timeout is not None else None
             try:
                 result = self.parent.switch()
                 if result is not self:
                     raise InvalidSwitchError('Invalid switch into Greenlet.get(): %r' % (result, ))
             finally:
+                if t is not None:
                     t.cancel()
         except:
             # unlinking in 'except' instead of finally is an optimization:

greentest/_import_wait.py

-# test__import_wait.py calls this
+# test__import_wait.py calls this via an import statement,
+# so all of this is happening with import locks held (especially on py2)
 import gevent
 
 
@@ -6,7 +7,20 @@ def fn2():
     return 2
 
 
+# A blocking function doesn't raise LoopExit
 def fn():
     return gevent.wait([gevent.spawn(fn2), gevent.spawn(fn2)])
 
-x = gevent.spawn(fn).get()
+gevent.spawn(fn).get()
+
+
+# Marshalling the traceback across greenlets doesn't
+# raise LoopExit
+def raise_name_error():
+    raise NameError("ThisIsExpected")
+
+try:
+    gevent.spawn(raise_name_error).get()
+    raise AssertionError("Should fail")
+except NameError as e:
+    x = e

greentest/test__import_wait.py

-# https://github.com/gevent/gevent/issues/652
+# https://github.com/gevent/gevent/issues/652 and 651
 from gevent import monkey
 monkey.patch_all()
 

greentest/testrunner.py

@@ -28,13 +28,10 @@ IGNORE_COVERAGE = [
     # Hangs forever
     'test__threading_vs_settrace.py',
     # XXX ?
-    'test__issue302monkey.py'
+    'test__issue302monkey.py',
+    "test_subprocess.py",
 ]
 
-if os.environ.get('TRAVIS'):
-    # Flaky on travis, unknown why
-    IGNORE_COVERAGE.append("test_subprocess.py")
-
 
 def run_many(tests, expected=(), failfast=False):
     global NWORKERS