On Jan 20, 2018, at 9:32 AM, Ilya Skriblovsky <ilyaskriblov...@gmail.com> wrote: > > Yes, doing it only for TLSMemoryBIOProtocol fails test too :( > > SSL-related seem to be touching both ends of this reference cycle after > connectionLost: > > 1. twisted/test/test_sslverify.py:2102 > self.assertEqual(sProto.wrappedProtocol.data, b'') > This one touches `wrappedProtocol` > > 2. twisted/test/proto_helpers.py:924 (waitUntilAllDisconnected, used by > twisted.web.test.test_webclient.WebClientSSLTests, for example) > if not True in [x.transport is not None and x.transport.connected for x in > protocols]: > and this one touches `transport` field > > There are other examples as well. > > Sure, these test failures can probably be fixed by changing tests themselves, > for example by making them to hold their own references to both wrapping and > wrapped protocols. But I'm not sure this wouldn't break any user's code > too... For my app it was easily fixed by breaking cycle in my protocol's > connectionLost. But I'm not experienced enough in Twisted internals to be > sure doing it inside TLSMemoryBIOProtocol wouldn't break any real-world usage > scenarios.
I think that this is worth trying, at least. If you could write a PR that fixes the tests, you might want to try following the exception process documented in https://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#procedure-for-exceptions-to-this-policy <https://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#procedure-for-exceptions-to-this-policy> and see if anyone has any code that might break. I'm pretty sure that the direction to break the cycle in is to break the reference to .wrappedProtocol, and not to mess with .wrappedProtocol.transport (which is not really something that should be touched from the outside of the wrapped protocol). -glyph
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
