Opened 15 years ago

Closed 11 years ago

#2642 defect closed wontfix (wontfix)

Circular reference in tcp.BaseClient

Reported by: mircea Owned by: luci
Priority: low Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, Thijs Triemstra, jesstess, khorn, ivank Branch:
Author:

Description

There is a circular reference created in the doConnect method of twisted.internet.tcp.BaseClient class. If the client fails to connect and exits the method calling failIfNotConnected, the references to self.doWrite and self.doRead (which were replaced with self.doConnect) are not deleted and this causes a circular reference.

I've attached a patch that fixes this by deleting the self.doWrite and self.doRead references on every doConnect method exit point.

Attachments (2)

circular_reference.diff (717 bytes) - added by mircea 15 years ago.
test_baseclient_leak.py (1.7 KB) - added by luci 13 years ago.

Download all attachments as: .zip

Change History (21)

Changed 15 years ago by mircea

Attachment: circular_reference.diff added

comment:1 Changed 15 years ago by therve

Resolution: wontfix
Status: newclosed

Circular references are handled without problems by these objects, because they don't have __del__ method. If you have a concrete problem, please reopen the ticket.

comment:2 Changed 13 years ago by luci

Resolution: wontfix
Status: closedreopened

This is a problem because it makes finding own circular references harder when other modules also create some. Having a garbage collector which is capable of handling circular references is not, in my opinnion, an excuse for introducing/not fixing them, especially if it's not hard to fix them. Any module which one writes that uses twisted.internet.tcp.BaseClient and wishes to fix circular references in, can be considered a concrete problem.

comment:3 Changed 13 years ago by Jean-Paul Calderone

Can you add unit tests for these changes?

comment:4 Changed 13 years ago by Glyph

Owner: changed from Glyph to luci
Status: reopenednew

I don't see it as an "excuse" for not fixing them - I see it as removing circular references from the list of things that need to be "fixed". Circular references are no more a bug than multiple inheritance or global variables; with a circular collector they're merely an implementation strategy which may be unsuitable in some cases.

Can you describe a situation in which circular references are actually a problem for you? Why you would want to remove them from your own code or from Twisted?

However, I'm not opposed to changing this. If you can supply some unit tests and a reason (even a very flimsy or very obscure one) why circular references are causing a problem for you, I'm sure that these changes can be integrated.

comment:5 Changed 13 years ago by Glyph

Priority: normallow

comment:6 Changed 13 years ago by luci

Hello and sorry for the long delay,

I've attached a unit test which tests whether such a leak occurs. As for the reason why I consider that circular references should be fixed: although the garbage collector can handle most (not all) circular references, it does more work while trying do so, thus being less efficient. While I do agree that in most cases this is certainly not a considerable problem, in some it may be. And some people do test their software to see whether they have such leaks.

I simply cannot understand how explicitly not breaking a circular reference (especially when doing so is easy) can be a design strategy.

comment:7 Changed 13 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

I simply cannot understand how explicitly not breaking a circular reference (especially when doing so is easy) can be a design strategy.

It's not that not breaking the cycles is a strategy; it's more that ensuring that cycles do not exist is probably infeasible. Cycles are, by their very nature (at least in Python), implicit. There's no object that represents a cycle; there isn't even a single reference that represents a cycle. There's a configuration of objects. How do you ensure that no configuration of objects ever exists such that there is a cycle? I can't think of a good way. So, we could break this cycle, but that doesn't mean there won't be other reference cycles in Twisted, nor does it mean that some other code path won't create a slightly different configuration of these very same objects which does have a reference cycle. The prospect we are looking at here is one of a never ending stream of trivial changes to any and all parts of Twisted to delete references which no automated approach to guaranteeing that the changes actually do any good.

So no one is against doing good, but there will be opposition to making lots of pointless changes. If these changes can be shown not to be pointless, then that's great and we should definitely make them. One hopes a unit test could serve this purpose, however...

The test you wrote looks fragile:

  • It might spuriously pass if the garbage collector runs on its own before the test gets around to invoking it.
  • It might spuriously fail if a different cycle gets collected, even if the cycle in BaseClient is fixed. This is particularly bad since it will fail in a way which gives no indication as to why it failed.
  • It's probably CPython specific (many tests that try to use the gc module fail terribly on other Python runtimes). This could be mitigated by only trying to run the test on CPython, of course, but presumably we'd want some way to guarantee the absence of a cycles on other Python runtimes (not having a test isn't the same as not having a failing test :).

Some of these concerns might be mitigated by using weak references instead of the relying on the return value of gc.collect(), but it will still probably require a degree of finesse.

To emphasize, this is not a rejection of the ticket or the change. It's just a caution against ineffective changes.

comment:8 Changed 13 years ago by luci

Hi,

I've uploaded a new unit test which takes the points into consideration. As for the initial comment: nobody said anything about all circular references in twisted, just about this one. If this one can be fixed, why not do it?

comment:9 Changed 13 years ago by Jean-Paul Calderone

Keywords: review added

comment:10 Changed 13 years ago by Jean-Paul Calderone

Owner: luci deleted

comment:11 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to luci

luci,

Thanks for persevering in your contribution :). Unfortunately, it still looks to me like the test is CPython-specific. By disabling GC, but stating that your object will be collected while the GC is disabled, you're assuming that the runtime performs reference counting as well as garbage collection. Certainly Jython does not do this. I don't believe PyPy does either.

Also, there's no need to actually invoke the reactor to test this circular reference. You can just create a BaseClient directly with a fake socket object, or create a subclass of Client and override createInternetSocket to return your fake socket. Then you don't have to worry about timing issues, which are already tweaky enough when one is interacting with GC.

Really the only API you can count on in the 'gc' module is gc.collect(). You can't depend on its return value, but you should be able to assume that weakrefs which are no longer referenced will die when it is invoked.

You might be able to write a test that is more runtime-agnostic by holding a weak reference to the offending bound method object, holding a strong reference to the BaseClient, and making sure that the bound method is released.

Or, you could add some platform-detection logic to make sure you're on CPython, and skip the tests on any other runtime, since that's where circular references are more interesting.

Changed 13 years ago by luci

Attachment: test_baseclient_leak.py added

comment:12 Changed 13 years ago by luci

Hi,

I've attached the test with a CPython detection mechanism stolen from python2.6's platform.python_implementation.

comment:13 Changed 13 years ago by luci

Owner: changed from luci to Glyph

Hi,

Since it's been over 20 days, I was curious whether you are going to take this ticket into consideration.

Thanks

comment:14 Changed 13 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Glyph deleted

Add the review keyword to tickets which are ready to have someone sign off on them. Otherwise we'll probably lose track of them in the huge pile of open tickets we have. :)

comment:15 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to luci

Luci,

Thanks again for your continued interest :). I have a bit more feedback on your contribution this time:

  1. The "_running_on_CPython" check would be better implemented as a method of twisted.python.runtime.Platform; putting it here in the middle of a test means the logic is likely to be duplicated, and it's unlikely we'll find bugs in i.e. its detection of IronPython if that changes. (For what it's worth, Twisted doesn't support IronPython and won't even import there, so you don't need to check for it.)
  2. As per our coding standard, all tests must have docstrings explaining what they're ensuring.
  3. If check_memory fails, the garbage collector will be left disabled. This logic should be in tearDown or a method passed to addCleanup, not in the flow of the test logic.
  4. Why bother with disabling gc and making the test python specific? You're already using a weakref; why not just hold part of the circular reference and call gc.collect() and ensure that the other part is collected? If the cycle isn't broken, even a proper collector won't get rid of it.
  5. As I said in my previous review, there's no need to actually invoke the reactor to test this circular reference. Tests which invoke the reactor tend to be flakier and fail due to unrelated timing- and network-related problems; we try hard to keep this kind of noise in our test results to a minimum.

Thanks again!

comment:16 Changed 12 years ago by Thijs Triemstra

Cc: Thijs Triemstra jesstess khorn added

comment:17 Changed 11 years ago by ivank

Cc: ivank added

circular_reference.diff applied to Twisted r30519 breaks test_clientConnectionFailedStopsReactor on an updated Ubuntu 10.04 64-bit:

$ /usr/bin/python ./bin/trial twisted.internet.test.test_tcp
[...]
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/elf/Projects/Twisted/twisted/internet/epollreactor.py", line 220, in _doReadOrWrite
    why = selectable.doWrite()
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 428, in doWrite
    result = abstract.FileDescriptor.doWrite(self)
  File "/home/elf/Projects/Twisted/twisted/internet/abstract.py", line 208, in doWrite
    l = self.writeSomeData(self.dataBuffer)
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 474, in writeSomeData
    return self.socket.send(buffer(data, 0, self.SEND_LIMIT))
exceptions.AttributeError: 'Client' object has no attribute 'socket'

twisted.internet.test.test_tcp.TCPClientTestsBuilder_EPollReactor.test_clientConnectionFailedStopsReactor
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/elf/Projects/Twisted/twisted/internet/gtk2reactor.py", line 294, in _doReadOrWrite
    why = source.doWrite()
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 428, in doWrite
    result = abstract.FileDescriptor.doWrite(self)
  File "/home/elf/Projects/Twisted/twisted/internet/abstract.py", line 208, in doWrite
    l = self.writeSomeData(self.dataBuffer)
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 474, in writeSomeData
    return self.socket.send(buffer(data, 0, self.SEND_LIMIT))
exceptions.AttributeError: 'Client' object has no attribute 'socket'

twisted.internet.test.test_tcp.TCPClientTestsBuilder_Glib2Reactor.test_clientConnectionFailedStopsReactor
twisted.internet.test.test_tcp.TCPClientTestsBuilder_Gtk2Reactor.test_clientConnectionFailedStopsReactor
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/elf/Projects/Twisted/twisted/internet/pollreactor.py", line 184, in _doReadOrWrite
    why = selectable.doWrite()
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 428, in doWrite
    result = abstract.FileDescriptor.doWrite(self)
  File "/home/elf/Projects/Twisted/twisted/internet/abstract.py", line 208, in doWrite
    l = self.writeSomeData(self.dataBuffer)
  File "/home/elf/Projects/Twisted/twisted/internet/tcp.py", line 474, in writeSomeData
    return self.socket.send(buffer(data, 0, self.SEND_LIMIT))
exceptions.AttributeError: 'Client' object has no attribute 'socket'

twisted.internet.test.test_tcp.TCPClientTestsBuilder_PollReactor.test_clientConnectionFailedStopsReactor
-------------------------------------------------------------------------------
Ran 72 tests in 0.249s

This *might* be hard to reproduce, because I never had this problem until I updated Ubuntu (I think).

$ uname -a
Linux hostname 2.6.32-27-server #49-Ubuntu SMP Thu Dec 2 02:05:21 UTC 2010 x86_64 GNU/Linux

comment:18 Changed 11 years ago by ivank

The offending line is the first del self.doWrite in the patch.

comment:19 Changed 11 years ago by Jean-Paul Calderone

Resolution: wontfix
Status: newclosed

There are many, many ways you can have cycles in Python. Most terribly, every single function in every module imported into a Python process participates in a reference cycle. The module has a __dict__, the __dict__ contains the functions, the functions have a func_globals which refers to the __dict__.

Note: See TracTickets for help on using tickets.