9374 Fixing circular references in t.p.policies and t.p.tls #955

IlyaSkriblovsky · 2018-01-27T20:25:45Z

https://twistedmatrix.com/trac/ticket/9374

ProtocolWrapper creates the circular reference between itself and its wrappedProtocol. Also TLSMemoryBIOProtocol creates the circular reference between itself and its _tlsConnection.

Both circles are never broken and still alive after connections are closed until garbage collector collects them.

This seems like a performance issue especially for apps handling many short-lived TLS connections. For default GC thresholds this will cause GC to be called very often wasting CPU time. For loosened GC thresholds (for the sake of performance) this leads to eating RAM very quickly.

Discussed in mailing list: https://twistedmatrix.com/pipermail/twisted-python/2018-January/031798.html

There is an associated ticket in Trac. Create a new one at https://twistedmatrix.com/trac/newticket
I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
I have updated the automated tests.

exarkun · 2018-01-27T22:24:07Z

This seems like a performance issue especially for apps handling many short-lived TLS connections. For default GC thresholds this will cause GC to be called very often wasting CPU time.

It doesn't matter how many cycles you actually have in your process: (on CPython) the garbage collector runs with the same frequency regardless, and does the same amount of work. The purpose of the garbage collector is to find the cycles among all the objects that exist in your process. It has slightly more work to do actually collecting the cycles that it finds but this is basically negligible.

For loosened GC thresholds (for the sake of performance) this leads to eating RAM very quickly.

Since GC CPU considerations are orthogonal, this is basically orthogonal too (that is, you may choose to tweak your GC settings but doing so has little or nothing to do with these cycles).

The difference that will be made is that some memory may be releasable to the OS slightly earlier if the cycles are broken earlier. In practice, I suspect it would be hard to observe this effect since memory is almost never released back to the OS anyway and over-commit means that unused memory is practically as good as released memory anyway.

IlyaSkriblovsky · 2018-01-28T11:00:43Z

Sorry, but I'm not agree that GC in CPython has fixed run frequency. gc.set_threshold manual says:

In order to decide when to run, the collector keeps track of the number object allocations and deallocations since the last collection. When the number of allocations minus the number of deallocations exceeds threshold0, collection starts.

So we can affect collection frequency by:

Setting higher GC thresholds. That's what my team does for cpu-intensive processes.
Carefully breaking unused reference cycles. By doing so, number of allocations minus the number of deallocations can be almost stable or growing very slowly and GC will be started rarely. But every single leaked reference cycle in a tight loop or in a frequently-repeated task (such as receiving tls connection) makes this value to grow rapidly. If this happen with loosened GC thresholds (see 1), and if reference cycle includes the protocol instance with heavy business-level data attached to it, the app's RAM usage can how quickly.

For example, I have the server that receives tons of short-lived TLS connections with loosened GC thresholds. Before this fix it took around 1.2Gb of RAM after 1 day after being started. With this fix it takes around 200Mb and not growing.

exarkun · 2018-01-28T11:43:50Z

Sorry, but I'm not agree that GC in CPython has fixed run frequency. gc.set_threshold manual says:

So it does. I had forgotten about that trait. Thanks for the doc reference and sorry about the incorrect post.

For example, I have the server that receives tons of short-lived TLS connections with loosened GC thresholds. Before this fix it took around 1.2Gb of RAM after 1 day after being started. With this fix it takes around 200Mb and not growing.

It would be great if you could contribute this as a repeatable benchmark.

IlyaSkriblovsky · 2018-01-28T17:02:55Z

Here is the benchmark script: https://gist.github.com/IlyaSkriblovsky/4dd3abfd5f67c64b13f1c673f56466f9

(the script is Py3-only because I don't know direct way to get collection count in Py2)

The script simulates 10k (set by N constant) TLS connections to connect-and-drop server. If loose cmdline arg it specified then it uses loosened GC thresholds. It prints out the number of times garbage collection was run and a peak RAM usage (using psutil).

Results on my machine:

GC thresholds	Wihout fix	With fix
default (700, 10, 10)	500 colls, 50mb	150 colls, 45mb
(100000, 200, 100)	3 colls, 288mb	1 coll, 89mb

So, with default GC settings there is small effect on RAM usage, but several times lower GC run count. With high GC thresholds there is significant difference in RAM usage.

rodrigc · 2018-01-29T16:22:00Z

@IlyaSkriblovsky please follow all the steps at http://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch . You've done most of the steps, but are missing step 11.

IlyaSkriblovsky · 2018-01-30T07:10:19Z

Did it, thanks

rodrigc · 2018-01-30T07:52:59Z

Looks like some of the earlier Windows failures with this PR were due to a broken version of pypiwin32: mhammond/pywin32#1151 . I triggered another build now that the broken version has been removed from pypiwin32

adiroiban · 2018-03-09T14:45:54Z

src/twisted/test/test_sslverify.py

+    clientWrappedProto = ListeningClient()
+    serverWrappedProto = GreetingServer()
+
+    cF = protocol.Factory()


I would prefer not to use abbreviations and call this clientFactory. ... why not clientFact ? :)

...same for clientWrappedProto ... why not cWP ?

I see that the existing code uses abbreviations, but I don't think that this is a good reason to continue to write new code in this way :)

adiroiban · 2018-03-09T14:50:01Z

src/twisted/protocols/tls.py

@@ -196,7 +198,9 @@ def makeConnection(self, transport):
        Connect this wrapper to the given transport and initialize the
        necessary L{OpenSSL.SSL.Connection} with a memory BIO.
        """
-        self._tlsConnection = self.factory._createConnection(self)
+        # Using weakref.proxy to avoid circular references
+        selfProxy = weakref.proxy(self)


just asking. Is using a weakref the only way to solve this?

Why not break it in connectionLost like done in the other place?

Using weakref can make the tests harder to read / write as you will need to take extra care on the scope of an instance..

...and I would like to write Python without having to care about the lifecycle of the objects.

adiroiban

Thanks for working on this.

Happy to see performance improvements :)

Is there a test for new the code in ProtocolWrapper ?

Can this be implemented without weakref?

I would like to see fewer (to none) abbreviations in the code and more docstrings, especially for the test part.

At 8AM and in the context of this PR the code is easy to ready. Hacking at 11PM and getting into some strange bugs, might prove that the code is not that easy to read.
Especially the dummy protocols and the Failure.trap in connectionLost

adiroiban · 2018-03-10T08:18:56Z

src/twisted/newsfragments/9374.bugfix

@@ -0,0 +1 @@
+t.p.p.ProtocolWrapper and t.p.t.TLSMemoryBIOProtocol no longer create circular references that still alive after connection is closed


please use full names and end the sentence with a full stop :)

adiroiban · 2018-03-10T08:19:39Z

src/twisted/protocols/policies.py

@@ -124,6 +124,9 @@ def connectionLost(self, reason):
        self.factory.unregisterProtocol(self)
        self.wrappedProtocol.connectionLost(reason)

+        # Breaking reference cycle between self and wrappedProtocol


Use full for a sentence.
In this way, it is clear that the sentence has an end and that it was not left unfinished by accident.

adiroiban · 2018-03-10T08:21:20Z

src/twisted/protocols/test/test_tls.py

@@ -42,7 +44,8 @@

 from twisted.internet.error import ConnectionDone, ConnectionLost
 from twisted.internet.defer import Deferred, gatherResults
-from twisted.internet.protocol import Protocol, ClientFactory, ServerFactory
+from twisted.internet.protocol import (Protocol, ClientFactory, ServerFactory,


this is fine, but to reduce future dif size, I would prefer to have this list alphabetically ordered and each import on a line.

from twisted.internet.protocol import ( ClientServerFactory, Factory, Protocol, ServerFactory, )

adiroiban · 2018-03-10T08:22:49Z

src/twisted/protocols/test/test_tls.py

+    def test_noCircularReferences(self):
+        """
+        TLSMemoryBIOProtocol doesn't leave circular references that make
+        it alive after connection is closed.


make it alive What does it mean?

adiroiban · 2018-03-10T08:24:46Z

src/twisted/protocols/test/test_tls.py

+        TLSMemoryBIOProtocol doesn't leave circular references that make
+        it alive after connection is closed.
+        """
+        def nObjectsOfType(type):


even if this is test code please consider adding a docstring to describe the purpose of this function.

There are no tests for the test code so in order to check whether the test code itself is correct we can only read the code.

adiroiban · 2018-03-10T08:26:23Z

src/twisted/protocols/test/test_tls.py

+        gc.disable()
+
+        try:
+            class DummyServerProtocol(Protocol):


Why is this protocol "dummy" ? What means dummy?

This looks like a fully working protocol which is closing the connecting first time it receives data.

I have used these types of protocol in production code :)

Please consider adding some docstring to document this code.

adiroiban · 2018-03-10T08:28:03Z

src/twisted/protocols/test/test_tls.py

+                    self.transport.loseConnection()
+
+
+            class DummyClientProtocol(Protocol):


Again, why dummy?
This looks like a fully functional protocol which will ignore all received data and will send a hello message on a connection.

adiroiban · 2018-03-10T08:29:38Z

src/twisted/protocols/test/test_tls.py

+                    self.transport.write(b'hello')
+
+                def connectionLost(self, reason):
+                    reason.trap(ConnectionDone)


Just asking. I have not executed (yet) the tests. Can the connection be closed for other reasons?
Please document why ConnectionDone is important here.

IlyaSkriblovsky · 2018-03-10T20:33:02Z

Thanks for review!
I've fixed some wording and naming issues.

I would be happy to explicitly break TLSMemoryBIOProtocol._tlsConnection reference cycle without weakrefs. But twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback scenario causes TLSMemoryBIOProtocol.dataReceived to be called after TLSMemoryBIOProtocol.connectionLost, causing null reference access if _tlsConnection is deleted in connectionLost. I've tried to dig into this to check whether this is a real-life case or a strange consequence of synthetic testing using IOPump. But the issue seems to be requiring deeper understanding of Twisted's TLS internals than I currently have :(

test_surpriseFromInfoCallback is the only test that failing if _tlsConnection is explicitly deleted in connectionLost.

adiroiban · 2018-03-10T20:54:08Z

src/twisted/newsfragments/9374.bugfix

@@ -0,0 +1 @@
+twisted.protocols.policies.ProtocolWrapper and twisted.protocols.tls.TLSMemoryBIOProtocol no longer create circular references that still alive after connection is closed.


references that still alive after :) please also update this part

adiroiban · 2018-03-10T21:13:18Z

But twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback scenario causes TLSMemoryBIOProtocol.dataReceived to be called after TLSMemoryBIOProtocol.connectionLost,

AFIK dataReceived should not be called after connectionLost.

@exarkun Can you please confirm if there should be any case in which dataReceived is called after connectionLost ?

I see this info in the API

class IProtocol(Interface):

    def dataReceived(data):
        """
        Called whenever data is received.

        Use this method to translate to a higher-level message.  Usually, some
        callback will be made upon the receipt of each complete protocol
        message.

        Please keep in mind that you will probably need to buffer some data
        as partial (or multiple) protocol messages may be received!  We
        recommend that unit tests for protocols call through to this method
        with differing chunk sizes, down to one byte at a time.

        @param data: bytes of indeterminate length
        @type data: L{bytes}
        """

    def connectionLost(reason):
        """
        Called when the connection is shut down.

        Clear any circular references here, and any external references
        to this Protocol.  The connection has been closed. The C{reason}
        Failure wraps a L{twisted.internet.error.ConnectionDone} or
        L{twisted.internet.error.ConnectionLost} instance (or a subclass
        of one of those).

        @type reason: L{twisted.python.failure.Failure}
        """

I can see that the API explicitly talks about cleaning circular references here, but does not make any statement about when dataReceived can be called... but since circularReferences are broken in connectionLost, I expect that this should be the last call for a certain instance.

The fact that test_surpriseFromInfoCallback fails might reveal a bug in the existing code.

I will give it a try and see why dataReceived is called after connectionLost in this test.

Thanks!

IlyaSkriblovsky · 2018-03-10T21:34:42Z

test_surpriseFromInfoCallback creates the server and the client on the top of twisted.test.iosim.FakeTransport transports. Then an exception is simulated during TLS handshake and one side calls his TLSMemoryBIOProtocol.failVerification() → abortConnection() → connectionLost(). But then another side gets its TLSMemoryBIOProtocol._flushSendBIO() called writing something to its transport. But FakeTransport stills receives data even when it is closed, so the written data successfully triggers dataReceived on the first side.

May be FakeTransport should check whether it is closed before accepting data in its write() method? If I put if self.disconnected: return as the first line of iosim.FakeTransport.write(), then all test seem to be passing with explicit deleting of _tlsConnection in connectionLost(). But I'm not sure that this is safe and reasonable thing to do.

Sorry if my explanations are confusing or inaccurate.

adiroiban · 2018-03-10T21:39:54Z

For the TLS/SSL connections there is one corner case which might get into the way of this patch... and this might be an existing bug... see https://github.com/twisted/twisted/blob/trunk/src/twisted/protocols/tls.py#L346

When you connect to a TLS/SSL socket and the connection is closed right away without sending any data, the connection is not actually closed.

I have checked the code and I see that connectionLost is called twice ...and before dataReceived, so I guess that there is a bug in iosim :)

IlyaSkriblovsky · 2018-03-10T21:59:55Z

connectionLost is called twice

It is probably called once for the server and once for the client. But then one of the sides calls _flushSendBIO and writes something to its transport (FakeTransport) and IOPump.pump() sends it into other side's dataReceived().

But I'm not sure where is the source of the bug: whether TLSMemoryBIOProtocol should not write to its transport after its connectionLost() called, or FakeTransport should not accept more write()s after it is marked as disconnected.

adiroiban · 2018-03-10T22:12:08Z

It is probably called once for the server and once for the client. But then one of the sides calls _flushSendBIO and writes something to its transport (FakeTransport) and IOPump.pump() sends it into other side's dataReceived().

True. I am stupid and confused :)

Let me dig more :)

Also, if I run a single tests, I see 14 errors reported by trial.

$ ./build/bin/trial twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback

SNIP

exceptions.ZeroDivisionError: division by zero

twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
-------------------------------------------------------------------------------
Ran 1 tests in 191.540s

FAILED (errors=14)

IlyaSkriblovsky · 2018-03-11T18:47:20Z

Also, if I run a single tests, I see 14 errors reported by trial.

That's because test_surpriseFromInfoCallback tests bogous function passed as info_callback to pyOpenSSL. And this callback seems to be called many times during TLS handshake. So the single test causes 14 deferred failures.

I like the idea of adding check for self.disconnected to iosim.FakeTransport.write(). This seems to solve the issue with nulled TLSMemoryBIOProtocol._tlsConnection in connectionLost() without breaking any other tests. But it would be cool if someone more experienced in Twisted internals can confirm that this is safe.

exarkun · 2018-03-11T20:20:53Z

I like the idea of adding check for self.disconnected to iosim.FakeTransport.write(). This seems to solve the issue with nulled TLSMemoryBIOProtocol._tlsConnection in connectionLost() without breaking any other tests. But it would be cool if someone more experienced in Twisted internals can confirm that this is safe.

All real ITransport implementations allow transport.loseConnection followed by transport.write or protocol.connectionLost followed by transport.write. In both these cases, they allow it and silently drop the write data on the floor. Since when a real connection has been lost the data can't be delivered, the data is not delivered in these cases.

It would be a good thing to make any test double for ITransport duplicate this behavior (on the premise that test doubles which do not replicate the actual behavior of the thing for which they are a double lead to false confidence in the test suite since they invalidate the tests they are used in by only telling you what the code under test would do if you used it against something different from the real implementation). Therefore, FakeTransport.write should indeed be changed to drop the data on the floor if called after the fake connection has been lost.

Three bonus points for also adding tests which demonstrate that FakeTransport and other ITransport implementations both behave this way (iow, expanded the surface of ITransport / FakeTransport which is "verified" by the test suite).

adiroiban · 2018-03-12T12:06:46Z

@IlyaSkriblovsky I see that you are member of Twisted team.
In this case please create branches directly in the main repo.
In this way, buildbot tests are automatically triggered and we can see the results from OSX.

Also, please consider writing tests for any code that you write, including tests for the testing infrastructure, as suggested by Jean Paul... and with the docstring to document that calling write after connection lost is acceptable :)

Thanks!

IlyaSkriblovsky · 2018-03-12T12:20:11Z

@adiroiban I'm member of Twisted team because I'm was contibuting to TxMongo, but I haven't ever contributed to Twisted itself. Okay, I will create branches in the main repo next time.

I will try to write tests for FakeTransport and other ITransport implementers.

adiroiban · 2018-03-12T16:42:22Z

I'm member of Twisted team because I'm was contributing to TxMongo

That is fine, you are good to create branches in Twisted :).
AFIK trunk is protected by GitHub, so you should not worry.

The main reason why we don't allow anybody to commit to the main repo, is that commits will trigger builds and we don't want to end up with PR which are executing malicious code on our buildslaves.

IlyaSkriblovsky · 2018-03-12T18:04:46Z

I've added a test for FakeTransport.write() dropping data after loseConnection().

Three bonus points for also adding tests which demonstrate that FakeTransport and other ITransport implementations both behave this way

Should I also find other fake ITransport implementations in testing code and add same tests (write after loseConnection) for them too? I can try, but it seems a bit outside of the scope of this particular PR.

adiroiban · 2018-03-12T21:04:17Z

Should I also find other fake ITransport implementations in testing code and add same tests (write after loseConnection) for them too? I can try, but it seems a bit outside of the scope of this particular PR.

I think that this can be done outside, so that we can merge this as it is.
A future ticket might also update the docstring for IProtocol to be explicit that is possible to have dataReceived after connectionLost

Thanks!

exarkun · 2018-03-16T12:24:58Z

src/twisted/protocols/test/test_tls.py

+            """
+            return sum(1 for x in gc.get_objects() if isinstance(x, type))
+
+        gc.disable()


fwiw you can get rid of the whole try/finally with self.addCleanup(gc.enable) right here.

Thanks, I forgot about it

IlyaSkriblovsky · 2018-03-16T14:18:20Z

When this happens, connectionLost should nevertheless only be called once. So it sounds like both epoll and asyncio reactors have a bug related to this?

Sorry, I was wrong. Two loseConnection() calls is simply one for a server and another for a client 🤦‍♂️
So the test failure is directly related to this PR: after we clear _tlsConnection in connectionLost, subsequent call to loseConnection will raise on attempting to access it. I will dig into it.

You can't do a null dereference in Python. You can attempt to access an attribute of an object which doesn't exist. This is a lot nicer than a null dereference.

You are right, sorry my C-ism. But to be even more presice, None is an object too! So it is attempt to access an attribute of existing object, but not the one you meant to :)

exarkun · 2018-03-16T14:22:29Z

You can't do a null dereference in Python. You can attempt to access an attribute of an object which doesn't exist. This is a lot nicer than a null dereference.

You are right, sorry my C-ism. But to be even more presice, None is an object too! So it is attempt to access an attribute of existing object, but not the one you meant to :)

Ah yes, sorry, that's what I meant but my statement was ambiguous (I meant the attribute doesn't exist on the object, not that the object doesn't exist 😄 ). Thanks for the clarification.

…otocol

glyph · 2018-03-17T20:54:00Z

Should this be back in review? It seems like all the blockers are dealt with but there’s a lot of discussion to review here :)

IlyaSkriblovsky · 2018-03-18T06:18:29Z

Yes, please review it again. I've added trivial check if TLSMemoryBIOProtocol already received `connectionLost()` in its `loseConnection()`. I hope it is reasonable check to do.

glyph · 2018-03-18T07:49:00Z

Sorry, I wasn't asking because I was about to review it :)

The place Twisted reviewers (such as yourself, congrats on your addition to the team :)) should look to see what needs reviewing is here:

https://twisted.reviews

Unfortunately this is what Trac is still required for, tracking this state. Please stick the 'review' keyword back on the Trac ticket so that it will show up there.

IlyaSkriblovsky · 2018-03-18T07:59:40Z

Sorry my silliness. Still getting familiar with the process. I've added "review" keyword and reassigned the ticket to nobody.

glyph · 2018-03-18T18:59:18Z

Sorry my silliness. Still getting familiar with the process. I've added "review" keyword and reassigned the ticket to nobody.

No worries. The process is definitely a bit clunky. One day somebody will have enough free time to migrate the rest of it to github and it won't require manually pushing so many buttons and twirling so many dials :)

adiroiban

Thanks a lot. Happy to see a green build :)
Looks good... but I think that it is missing a few tests.

Other than that, it should be ready for merging soon :)

adiroiban · 2018-03-18T22:38:04Z

src/twisted/protocols/tls.py


    def loseConnection(self):
        """
        Send a TLS close alert and close the underlying connection.
        """
-        if self.disconnecting:
+        if self.disconnecting or not self.connected:


is there a test for this change? :)

adiroiban · 2018-03-18T22:42:12Z

src/twisted/protocols/policies.py

@@ -124,6 +124,9 @@ def connectionLost(self, reason):
        self.factory.unregisterProtocol(self)
        self.wrappedProtocol.connectionLost(reason)

+        # Breaking reference cycle between self and wrappedProtocol.
+        self.wrappedProtocol = None


do we have tests for this change?

Besides updating the existing tests for policies, I was expecting a new test (or update of an existing test) for connectionLost in which of which the docstring describe that on connection lost the wrappedProtocol is None

adiroiban · 2018-03-24T10:59:12Z

@IlyaSkriblovsky before pushing changes to this, please merge with trunk.
This will get the Circle-CI configuration.

Also, before pushing a code, try to run pyflakes and pycodestyle on local code to reduce the load on our testing servers.... Travis and Appveyor are free... but in the end, somebody has to pay for their usage

IlyaSkriblovsky · 2018-03-24T11:01:59Z

@adiroiban you are right, sorry. Will do as you said. I've cancelled Travis build, but I can't cancel AppVeyor.

…ky-circular-refs-in-tls

IlyaSkriblovsky · 2018-03-24T13:17:24Z

@adiroiban test failure looks like unrelated to this PR because all buildbot builds was green until today changes, but today I only added some tests without changing implementation. Is it possible to restart this particular build to check if failure is permanent?

adiroiban · 2018-03-24T13:44:53Z

@IlyaSkriblovsky I have restarted it.

You can also restart it... check for credentials here https://twistedmatrix.com/trac/wiki/ContinuousIntegration/DeveloperWorkflow#WriteAccess :)

adiroiban · 2018-03-24T13:46:52Z

but that is a flaky test... I saw it failing in other branches

IlyaSkriblovsky · 2018-03-24T14:02:00Z

Thank you. I'm going to put this back in review.

adiroiban

Looks good.

My only minor comment is about the short cF, sF, serverWrappedProto, cProto ... ex why cProto and not cP ... or why cF and not cFactory or cFact ...

I would like to see non-abreviated names... but I will not block the merge on that :)

Feel free to merge :)

Thanks a lot for the great work!

…ky-circular-refs-in-tls

IlyaSkriblovsky · 2018-03-25T14:37:27Z

I've renamed cF and sF to clientFactory and serverFactory. I've just used cF and sF because saw these names in some another place with analogous code. But you are right, this is not justifying reason :)

So, waiting for testing results after updating to trunk and merging.

IlyaSkriblovsky · 2018-03-25T20:27:21Z

I'm not sure why this build failed: https://buildbot.twistedmatrix.com/builders/osx10.10-py2.7/builds/1350
It can't install tox!? But this build successfully installed it: https://buildbot.twistedmatrix.com/builders/osx10.10-py2.7-coverage/builds/1340

What is going on?
This is certainly not in the scope of this PR, but I'm a bit afraid of breaking the trunk.

adiroiban · 2018-03-25T20:29:42Z

@IlyaSkriblovsky feel free to merge it. This is a known issue (see the conversation from the mailing list)

It was green before...but while testing the OSX built, I picked your branch for testing. Sorry :)

The OSX is executed over circle-ci so we are covered.

Thanks!

IlyaSkriblovsky · 2018-03-25T21:06:57Z

@adiroiban Done, thanks!

rodrigc closed this Jan 30, 2018

rodrigc reopened this Jan 30, 2018

adiroiban reviewed Mar 9, 2018

View reviewed changes

adiroiban requested changes Mar 10, 2018

View reviewed changes

adiroiban reviewed Mar 10, 2018

View reviewed changes

IlyaSkriblovsky added 4 commits March 13, 2018 20:16

fixing circular references in t.p.policies and t.p.tls

2f6e58a

not assuming zero initial object counters

82bfade

code style fixes

c251093

docstrings, variable naming, code style

e073b04

exarkun reviewed Mar 16, 2018

View reviewed changes

nicer cleanup

5dda60a

allow no-op loseConnection() after connectionLost() in TLSMemoryBIOPr…

127fa5a

…otocol

adiroiban requested changes Mar 18, 2018

View reviewed changes

Tests for changed behavior of ProtocolWrapper and TLSMemoryBIOProtocol

7e94744

IlyaSkriblovsky added 2 commits March 24, 2018 14:29

test rewritten without inlineCallbacks

7809113

Merge remote-tracking branch 'upstream/trunk' into 9374-ilyaskriblovs…

d45dc01

…ky-circular-refs-in-tls

adiroiban approved these changes Mar 24, 2018

View reviewed changes

IlyaSkriblovsky added 2 commits March 25, 2018 17:30

abbreviated names expanded

b0433dd

Merge remote-tracking branch 'upstream/trunk' into 9374-ilyaskriblovs…

a55a8b0

…ky-circular-refs-in-tls

IlyaSkriblovsky merged commit a55a8b0 into twisted:trunk Mar 25, 2018

IlyaSkriblovsky deleted the 9374-ilyaskriblovsky-circular-refs-in-tls branch March 26, 2018 06:22

Ksianka mentioned this pull request Jun 2, 2019

Document that Mailsender.send() returns a Deferred scrapy/scrapy#3478

Open

		@@ -0,0 +1 @@
		t.p.p.ProtocolWrapper and t.p.t.TLSMemoryBIOProtocol no longer create circular references that still alive after connection is closed

		self.transport.loseConnection()


		class DummyClientProtocol(Protocol):

		@@ -0,0 +1 @@
		twisted.protocols.policies.ProtocolWrapper and twisted.protocols.tls.TLSMemoryBIOProtocol no longer create circular references that still alive after connection is closed.

9374 Fixing circular references in t.p.policies and t.p.tls #955

9374 Fixing circular references in t.p.policies and t.p.tls #955

Conversation

IlyaSkriblovsky commented Jan 27, 2018 • edited Loading

exarkun commented Jan 27, 2018

IlyaSkriblovsky commented Jan 28, 2018

exarkun commented Jan 28, 2018

IlyaSkriblovsky commented Jan 28, 2018 • edited Loading

rodrigc commented Jan 29, 2018

IlyaSkriblovsky commented Jan 30, 2018

rodrigc commented Jan 30, 2018

Choose a reason for hiding this comment

adiroiban Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaSkriblovsky commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Mar 10, 2018

IlyaSkriblovsky commented Mar 10, 2018 • edited Loading

adiroiban commented Mar 10, 2018

IlyaSkriblovsky commented Mar 10, 2018

adiroiban commented Mar 10, 2018

IlyaSkriblovsky commented Mar 11, 2018

exarkun commented Mar 11, 2018

adiroiban commented Mar 12, 2018

IlyaSkriblovsky commented Mar 12, 2018 • edited Loading

adiroiban commented Mar 12, 2018

IlyaSkriblovsky commented Mar 12, 2018

adiroiban commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaSkriblovsky commented Mar 16, 2018

exarkun commented Mar 16, 2018

glyph commented Mar 17, 2018

IlyaSkriblovsky commented Mar 18, 2018 via email • edited Loading

glyph commented Mar 18, 2018

IlyaSkriblovsky commented Mar 18, 2018

glyph commented Mar 18, 2018

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Mar 24, 2018

IlyaSkriblovsky commented Mar 24, 2018

IlyaSkriblovsky commented Mar 24, 2018

adiroiban commented Mar 24, 2018

adiroiban commented Mar 24, 2018

IlyaSkriblovsky commented Mar 24, 2018 • edited Loading

adiroiban left a comment

Choose a reason for hiding this comment

IlyaSkriblovsky commented Mar 25, 2018

IlyaSkriblovsky commented Mar 25, 2018

adiroiban commented Mar 25, 2018

IlyaSkriblovsky commented Mar 25, 2018

IlyaSkriblovsky commented Jan 27, 2018 •

edited

Loading

IlyaSkriblovsky commented Jan 28, 2018 •

edited

Loading

adiroiban Mar 9, 2018 •

edited

Loading

IlyaSkriblovsky commented Mar 10, 2018 •

edited

Loading

IlyaSkriblovsky commented Mar 12, 2018 •

edited

Loading

IlyaSkriblovsky commented Mar 18, 2018 via email •

edited

Loading

IlyaSkriblovsky commented Mar 24, 2018 •

edited

Loading