Stack overflow in gen.py

[bdrung]

I am trying to build salt 2018.3.3 against tornado 4.5.3 for Debian. tornado 4.5.3 builds with the test succeeded (except four, see #2536) after renaming it from tornado to tornado4 (to make it co-installable). The test suite for salt fails:
https://launchpadlibrarian.net/401405967/buildlog_ubuntu-disco-amd64.salt_2018.3.3+dfsg1-1~wip20181213~ubuntu19.04.1~ppa1_BUILDING.txt.gz

Current thread 0x00007f1f5d3bc740 (most recent call first):
  File "/usr/lib/python3.7/abc.py", line 139 in __instancecheck__
  File "/usr/lib/python3.7/inspect.py", line 226 in isawaitable
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 1279 in convert_yielded
  File "/usr/lib/python3.7/functools.py", line 824 in wrapper
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 1142 in handle_yield
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 1000 in __init__
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 319 in wrapper
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 1280 in convert_yielded
[....]
  File "/usr/lib/python3.7/functools.py", line 824 in wrapper
  File "/usr/lib/python3/dist-packages/tornado4/gen.py", line 1142 in handle_yield
  ...

Hereby I request help for debugging the issue. The stacktrace points to tornado as culprit. Here are the changes that I did to it: https://salsa.debian.org/python-team/modules/python-tornado/commits/tornado4

[bdarnell]

FWIW I'd much rather spend time helping salt get onto tornado 5 than debugging issues in older versions.

Can I see the fully-patched source somewhere? The linked patch doesn't change any line numbers but apparently some other stuff in the debian patch series does because those line numbers don't line up.

The regular tornado package is not even installed during these test runs, right? If it's installed I'd throw in an assert 'tornado' not in sys.modules to make sure nothing escaped the import renaming.

I remember seeing this infinite recursion while working on 5.x, and it was related to a call to tornado.gen.convert_yielded.register being missing. This was most often the one in tornado.platform.asyncio, because the import of that module was conditional in 4.5; it became unconditional in 5.0 for this reason. Try adding an import of tornado4.platform.asyncio to the end of tornado4.ioloop.

If that's not it, I'd either get these tests running in a debugger or add some debug prints to convert_yielded:

seen_types = set()

def convert_yielded(yielded):
    t = type(yielded)
    if t not in seen_types:
        print('convert_yielded:', t)
        seen_types.add(t)
    ...

[bdrung]

I would love to have salt working with tornado 5, but according to saltstack/salt-ci-images#995 there is a lot of work left to do. Debian testing has a soft freeze on 2019-02-12 for the next release. Until then I like to see a working salt package in Debian (upload to unstable needs to happen several days before, let's say end of January). I doubt that the tornado 5 support will be ready until then. That's why I took the path to use tornado 4.

https://salsa.debian.org/python-team/modules/python-tornado/commits/tornado4 and https://salsa.debian.org/salt-team/salt/tree/wip-2018.3 contain the full source. There are quilt patches in debian/patches that are applied before the package is built. So please look there. You can either use quilt or patch -p1 to apply those patches.

The package are built in a minimal chroot environment. So the regular tornado package is not installed. So nothing escaped the import renaming. I will retry the tests without the rename to verify that the rename does not introduce a regression.

[bdrung]

I applied this requested patch and rerun the salt package build:

--- python-tornado4-4.5.3.orig/tornado/ioloop.py
+++ python-tornado4-4.5.3/tornado/ioloop.py
@@ -1039,3 +1039,6 @@ class PeriodicCallback(object):
                                                   callback_time_sec) + 1) * callback_time_sec
 
             self._timeout = self.io_loop.add_timeout(self._next_timeout, self._run)
+
+# Try fixing https://github.com/tornadoweb/tornado/issues/2553
+import tornado4.platform.asyncio

The salt test still fail. I'll add the debug prints that you suggested and try again.

[bdrung]

I tested building Python against tornado 4 without the rename and there the test succeeded (except one failing for kubernetes). So this endless loop is caused by the rename. May this construct causes it:

try:
    import tornado4
    import sys
    sys.modules['tornado'] = tornado4
except ImportError:
    pass

[bdrung]

Using

try:
    import tornado4
    import sys
    sys.modules['tornado'] = tornado4
except ImportError:
    pass

caused this issue. Using constructs like

try:
    from tornado4.ioloop import IOLoop
    from tornado4.iostream import IOStream, SSLIOStream
except ImportError:
    from tornado.ioloop import IOLoop
    from tornado.iostream import IOStream, SSLIOStream

works, but requires more lines to be modified.

[bdarnell]

Ah, that's interesting. I think there are some ways around it (for example, importing all the submodules of tornado before doing the sys.modules assignment), but in any case it's good to have something that works.