Skip to content

Commit a192634

Browse files
authored
Merge pull request #3315 from bdarnell/circlerefs
ioloop,concurrent: Fix reference cycles
2 parents cbe70c2 + 6f89211 commit a192634

File tree

5 files changed

+221
-113
lines changed

5 files changed

+221
-113
lines changed

maint/circlerefs/circlerefs.py

Lines changed: 0 additions & 107 deletions
This file was deleted.

tornado/concurrent.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,7 @@ def chain_future(a: "Future[_T]", b: "Future[_T]") -> None:
150150
151151
"""
152152

153-
def copy(future: "Future[_T]") -> None:
154-
assert future is a
153+
def copy(a: "Future[_T]") -> None:
155154
if b.done():
156155
return
157156
if hasattr(a, "exc_info") and a.exc_info() is not None: # type: ignore

tornado/ioloop.py

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -696,15 +696,13 @@ def add_future(
696696
# the error logging (i.e. it goes to tornado.log.app_log
697697
# instead of asyncio's log).
698698
future.add_done_callback(
699-
lambda f: self._run_callback(functools.partial(callback, future))
699+
lambda f: self._run_callback(functools.partial(callback, f))
700700
)
701701
else:
702702
assert is_future(future)
703703
# For concurrent futures, we use self.add_callback, so
704704
# it's fine if future_add_done_callback inlines that call.
705-
future_add_done_callback(
706-
future, lambda f: self.add_callback(callback, future)
707-
)
705+
future_add_done_callback(future, lambda f: self.add_callback(callback, f))
708706

709707
def run_in_executor(
710708
self,

tornado/test/circlerefs_test.py

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
"""Test script to find circular references.
2+
3+
Circular references are not leaks per se, because they will eventually
4+
be GC'd. However, on CPython, they prevent the reference-counting fast
5+
path from being used and instead rely on the slower full GC. This
6+
increases memory footprint and CPU overhead, so we try to eliminate
7+
circular references created by normal operation.
8+
"""
9+
10+
import asyncio
11+
import contextlib
12+
import gc
13+
import io
14+
import sys
15+
import traceback
16+
import types
17+
import typing
18+
import unittest
19+
20+
import tornado
21+
from tornado import web, gen, httpclient
22+
from tornado.test.util import skipNotCPython
23+
24+
25+
def find_circular_references(garbage):
26+
"""Find circular references in a list of objects.
27+
28+
The garbage list contains objects that participate in a cycle,
29+
but also the larger set of objects kept alive by that cycle.
30+
This function finds subsets of those objects that make up
31+
the cycle(s).
32+
"""
33+
34+
def inner(level):
35+
for item in level:
36+
item_id = id(item)
37+
if item_id not in garbage_ids:
38+
continue
39+
if item_id in visited_ids:
40+
continue
41+
if item_id in stack_ids:
42+
candidate = stack[stack.index(item) :]
43+
candidate.append(item)
44+
found.append(candidate)
45+
continue
46+
47+
stack.append(item)
48+
stack_ids.add(item_id)
49+
inner(gc.get_referents(item))
50+
stack.pop()
51+
stack_ids.remove(item_id)
52+
visited_ids.add(item_id)
53+
54+
found: typing.List[object] = []
55+
stack = []
56+
stack_ids = set()
57+
garbage_ids = set(map(id, garbage))
58+
visited_ids = set()
59+
60+
inner(garbage)
61+
return found
62+
63+
64+
@contextlib.contextmanager
65+
def assert_no_cycle_garbage():
66+
"""Raise AssertionError if the wrapped code creates garbage with cycles."""
67+
gc.disable()
68+
gc.collect()
69+
gc.set_debug(gc.DEBUG_STATS | gc.DEBUG_SAVEALL)
70+
yield
71+
try:
72+
# We have DEBUG_STATS on which causes gc.collect to write to stderr.
73+
# Capture the output instead of spamming the logs on passing runs.
74+
f = io.StringIO()
75+
old_stderr = sys.stderr
76+
sys.stderr = f
77+
try:
78+
gc.collect()
79+
finally:
80+
sys.stderr = old_stderr
81+
garbage = gc.garbage[:]
82+
# Must clear gc.garbage (the same object, not just replacing it with a
83+
# new list) to avoid warnings at shutdown.
84+
gc.garbage[:] = []
85+
if len(garbage) == 0:
86+
return
87+
for circular in find_circular_references(garbage):
88+
f.write("\n==========\n Circular \n==========")
89+
for item in circular:
90+
f.write(f"\n {repr(item)}")
91+
for item in circular:
92+
if isinstance(item, types.FrameType):
93+
f.write(f"\nLocals: {item.f_locals}")
94+
f.write(f"\nTraceback: {repr(item)}")
95+
traceback.print_stack(item)
96+
del garbage
97+
raise AssertionError(f.getvalue())
98+
finally:
99+
gc.set_debug(0)
100+
gc.enable()
101+
102+
103+
# GC behavior is cpython-specific
104+
@skipNotCPython
105+
class CircleRefsTest(unittest.TestCase):
106+
def test_known_leak(self):
107+
# Construct a known leak scenario to make sure the test harness works.
108+
class C(object):
109+
def __init__(self, name):
110+
self.name = name
111+
self.a: typing.Optional[C] = None
112+
self.b: typing.Optional[C] = None
113+
self.c: typing.Optional[C] = None
114+
115+
def __repr__(self):
116+
return f"name={self.name}"
117+
118+
with self.assertRaises(AssertionError) as cm:
119+
with assert_no_cycle_garbage():
120+
# a and b form a reference cycle. c is not part of the cycle,
121+
# but it cannot be GC'd while a and b are alive.
122+
a = C("a")
123+
b = C("b")
124+
c = C("c")
125+
a.b = b
126+
a.c = c
127+
b.a = a
128+
b.c = c
129+
del a, b
130+
self.assertIn("Circular", str(cm.exception))
131+
# Leading spaces ensure we only catch these at the beginning of a line, meaning they are a
132+
# cycle participant and not simply the contents of a locals dict or similar container. (This
133+
# depends on the formatting above which isn't ideal but this test evolved from a
134+
# command-line script) Note that the behavior here changed in python 3.11; in newer pythons
135+
# locals are handled a bit differently and the test passes without the spaces.
136+
self.assertIn(" name=a", str(cm.exception))
137+
self.assertIn(" name=b", str(cm.exception))
138+
self.assertNotIn(" name=c", str(cm.exception))
139+
140+
async def run_handler(self, handler_class):
141+
app = web.Application(
142+
[
143+
(r"/", handler_class),
144+
]
145+
)
146+
socket, port = tornado.testing.bind_unused_port()
147+
server = tornado.httpserver.HTTPServer(app)
148+
server.add_socket(socket)
149+
150+
client = httpclient.AsyncHTTPClient()
151+
with assert_no_cycle_garbage():
152+
# Only the fetch (and the corresponding server-side handler)
153+
# are being tested for cycles. In particular, the Application
154+
# object has internal cycles (as of this writing) which we don't
155+
# care to fix since in real world usage the Application object
156+
# is effectively a global singleton.
157+
await client.fetch(f"http://127.0.0.1:{port}/")
158+
client.close()
159+
server.stop()
160+
socket.close()
161+
162+
def test_sync_handler(self):
163+
class Handler(web.RequestHandler):
164+
def get(self):
165+
self.write("ok\n")
166+
167+
asyncio.run(self.run_handler(Handler))
168+
169+
def test_finish_exception_handler(self):
170+
class Handler(web.RequestHandler):
171+
def get(self):
172+
raise web.Finish("ok\n")
173+
174+
asyncio.run(self.run_handler(Handler))
175+
176+
def test_coro_handler(self):
177+
class Handler(web.RequestHandler):
178+
@gen.coroutine
179+
def get(self):
180+
yield asyncio.sleep(0.01)
181+
self.write("ok\n")
182+
183+
asyncio.run(self.run_handler(Handler))
184+
185+
def test_async_handler(self):
186+
class Handler(web.RequestHandler):
187+
async def get(self):
188+
await asyncio.sleep(0.01)
189+
self.write("ok\n")
190+
191+
asyncio.run(self.run_handler(Handler))
192+
193+
def test_run_on_executor(self):
194+
# From https://github.com/tornadoweb/tornado/issues/2620
195+
#
196+
# When this test was introduced it found cycles in IOLoop.add_future
197+
# and tornado.concurrent.chain_future.
198+
import concurrent.futures
199+
200+
thread_pool = concurrent.futures.ThreadPoolExecutor(1)
201+
202+
class Factory(object):
203+
executor = thread_pool
204+
205+
@tornado.concurrent.run_on_executor
206+
def run(self):
207+
return None
208+
209+
factory = Factory()
210+
211+
async def main():
212+
# The cycle is not reported on the first call. It's not clear why.
213+
for i in range(2):
214+
await factory.run()
215+
216+
with assert_no_cycle_garbage():
217+
asyncio.run(main())

tornado/test/runtests.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
"tornado.test.asyncio_test",
2323
"tornado.test.auth_test",
2424
"tornado.test.autoreload_test",
25+
"tornado.test.circlerefs_test",
2526
"tornado.test.concurrent_test",
2627
"tornado.test.curl_httpclient_test",
2728
"tornado.test.escape_test",

0 commit comments

Comments
 (0)