Ticket #3810 regression closed fixed

Opened 5 years ago

Last modified 5 years ago

trial -u leaks memory

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: trial Keywords:
Cc: Branch: branches/stop-thread-pool-3810
(diff, github, buildbot, log)
Author: exarkun, radix Launchpad Bug:

Description

This can be observed by running the following test suite:

from twisted.trial.unittest import TestCase

class Empty(TestCase):
    def test_empty(self):
        pass

like this:

exarkun@charm:/tmp$ trial -u test_empty.py

The cause seems to be some additional (#3786) threadpool hackery in trial.

Attachments

spike.patch Download (1.6 KB) - added by exarkun 5 years ago.

Change History

Changed 5 years ago by exarkun

1

Changed 5 years ago by exarkun

Attached is a patch which demonstrates the minimum change required to fix the leak. Clearly it is not actually suitable to be applied as-is.

2

Changed 5 years ago by exarkun

  • branch set to branches/stop-thread-pool-3810
  • branch_author set to exarkun

(In [26782]) Branching to 'stop-thread-pool-3810'

3

Changed 5 years ago by radix

  • owner jml deleted
  • keywords review added

This is ready for review.

4

Changed 5 years ago by glyph

  • keywords review removed
  • owner set to exarkun

Pretty much every test in the entire suite now produces this error:

Traceback (most recent call last):
  File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 967, in _classCleanUp
    util._Janitor(self, result).postClassCleanup()
  File "/home/glyph/Projects/Twisted/trunk/twisted/trial/util.py", line 150, in postClassCleanup
    self._cleanThreads()
  File "/home/glyph/Projects/Twisted/trunk/twisted/trial/util.py", line 193, in _cleanThreads
    reactor._stopThreadPool()
  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/base.py", line 926, in _stopThreadPool
    self.threadpool.stop()
exceptions.AttributeError: 'NoneType' object has no attribute 'stop'

they all also say [OK] so ... hrm.

5

Changed 5 years ago by exarkun

(In [26816]) re-add the check of reactor.threadpool being None to avoid stopping a threadpool which doesn't exist

refs #3810

6

Changed 5 years ago by exarkun

  • owner exarkun deleted
  • keywords review added
  • branch_author changed from exarkun to exarkun, radix

r26816 fixes the problem. The cleanup code was trying to destroy the threadpool unconditionally. If a test method didn't ever trigger the creation of a threadpool, as most don't, this would result in the reported AttributeError.

7

Changed 5 years ago by therve

  • keywords review removed
  • owner set to exarkun

*

+        When L{ReactorBase._stopThreadPool} drops the reactor's direct
+        reference to its internal threadpool and removes the associated startup 
+        and shutdown triggers.

I think the sentence is missing something.

  • Pyflakes:
    twisted/internet/test/test_base.py:8: 'gc' imported but unused
    twisted/internet/test/test_base.py:10: 'ref' imported but unused
    twisted/internet/test/test_base.py:17: 'ReactorBase' imported but unused
    

Please merge once fixed. Thanks!

8

Changed 5 years ago by exarkun

(In [26858]) Address review feedback

  • fix test_stopThreadPool docstring
  • remove unused imports from test_base.py

refs #3810

9

Changed 5 years ago by exarkun

(In [26860]) Update copyright date

refs #3810

10

Changed 5 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [26861]) Merge stop-thread-pool-3810

Author: radix, exarkun Reviewer: glyph, therve Fixes: #3810

Change the interaction between trial and the reactor in the area of threadpool cleanup. Move more responsibility into the reactor so that trial is doing (very) slightly less to objects it isn't really responsible for.

Specifically, make the reactor's threadpool cleanup method also clean up the startup trigger it adds to itself, if necessary, and make trial only call the reactor's threadpool cleanup method when it wants to clean up threads.

Also add test coverage for the reactor's threadpool cleanup code for each of the various states it is possible for the reactor to be in.

11

Changed 3 years ago by <automation>

  • owner exarkun deleted
Note: See TracTickets for help on using tickets.