Opened 6 years ago

Closed 5 years ago

#3810 regression closed fixed (fixed)

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 (1)

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

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by exarkun

comment:1 Changed 6 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.

comment:2 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/stop-thread-pool-3810

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

comment:3 Changed 6 years ago by radix

  • Keywords review added
  • Owner jml deleted

This is ready for review.

comment: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.

comment: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

comment:6 Changed 5 years ago by exarkun

  • Author changed from exarkun to exarkun, radix
  • Keywords review added
  • Owner exarkun deleted

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.

comment: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!

comment: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

comment:9 Changed 5 years ago by exarkun

(In [26860]) Update copyright date

refs #3810

comment:10 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.

comment:11 Changed 4 years ago by <automation>

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