Ensure connections are properly closed on objects destruction

[vincentxb]

This fixes the issue raised by @tardyp on https://github.com/kennethreitz/requests/issues/239.

We have not been able to reproduce the scenario on a simple testcase. However, in our environment involving Buildbot, Twisted, and txrequests, the number of connections in a CLOSE_WAIT state grows until we reach the maximum open files limit per process!

While it's still obscure to me what would prevent an HTTPConnection object from being destroyed once it's no longer used, this fix appears to work in our environment, at least we no longer see this CLOSE_WAIT connections leak...

[piotr-dobrogost]

Related issues: #88 and #132

[shazow]

Hrm, not ideal but I guess there's no harm. /cc @t-8ch @Lukasa in case you have any better ideas. :)

[Lukasa]

This fix bothers me. It's not a bad idea, it certainly feels fine, but I can't see why it would help. We don't delete anything like this anywhere in Requests or urllib3, at least not as far as I can see. Does the garbage collector call__del__? Surely not. I'll investigate.

[Lukasa]

Urg, in fact, the GC does. Well that's annoying. Well damn. @vincentbesanceney, is it possible for you to try to reduce the leak to the smallest number of these ``del` additions? It would be interesting to know exactly what we're leaking.

[tardyp]

Vincent can try that tomorrow, but from what I understand, the collection's clear() does not call close for each their elements, but rather delete them (or loose a ref).
So my bet would be on the connectionpool.py change

[Lukasa]

Mm, that seems most plausible to me too. It would be best if we can restrict this change to the minimal number of changes required to fix the problem, because the GC gets sad if there's a reference cycle between objects that have __del__ methods. Best not to make that easy to do.

[shazow]

+1. Please send an updated PR once we figure out the minimum changes required for everyone to be happy. :)

[sigmavirus24]

I see no tests to show that this will not regress as it seems it has already done so once. "Won't someone think of the tests?!"

[shazow]

Yar, tests would be nice. It's hard to reproduce tests in platform-specific environments so I'm a little less stringent in requiring full coverage on patches in that context, but is still very much desirable.

Btw @sigmavirus24 @Lukasa, let me know if you guys feel this should be reverted until the improvements above are done. I can go either way.

[sigmavirus24]

Tests would be great if only to ensure that the API doesn't revert (e.g., that calling del obj actually calls obj.close() or whatever; do we have mock as a test dependency here?).

That said, I think adding this in three places is stinky but for now okay. Whichever person takes up the real fix though should include a commit to revert this work entirely though. It will be unnecessary if they can make a change in one place exactly.

This can be a short-term stop-gap solution but I don't like it.

[sigmavirus24]

I answered my question about mock on my own. I'm going to take a crack at this tonight. Maybe finally get a PR into urllib3. 📦

[shazow]

I rather not have tests which just test the presence of some code, rather than the desired underlying behaviour and problem we're trying to avoid.

[sigmavirus24]

It's going to be hard trying to reproduce hitting the ulimit on a machine unless we somehow* lower it ourselves.

• we can set ulimit ourselves before running the tests but I dislike that immensely

[shazow]

Alright, this PR is reverted. Let's try again, with less breaking and more of the stuff we discussed. :D

[vincentxb]

Greetings all, I'll keep you posted once I get the bottom of it. From what I see, my pull request caused more trouble than it was worth, sorry for that.

[sigmavirus24]

@vincentbesanceney I'm working on it now too. Do you have experience remote pairing? If so we could collaborate on this one night this (or next) week.

[vincentxb]

@sigmavirus24 Well, I don't want to put you on the wrong track. As I said in my pull request message, it was obscure to me what would prevent an HTTPConnection object from being destroyed once it's no longer used. I didn't see in urllib3 any culrpit. So, my fix was more a safe bet that it wouldn't hurt to close connections on object destruction for objects that were referencing the connections.

I actually found two culprits in our own code, the most tricky one was a cell object referencing a session object. I still want to make sure that I eradicate the issue completely, but my last findings tend to demonstrate that urllib3 has nothing to do with it. For this, I apologize.

Anyway, I have no experience in remote pairing, but I'd love to learn.

[sigmavirus24]

I submitted #294 to hopefully fix what should be the only possible culprit here. That said, I was going to look more closely at what txrequests does under the covers to see if they're passing something like stream=True in which case if you don't consume all of the data then you will never close the connection.

A undead reference to a session object could do it too.

And remote pairing would still be cool but maybe not on this.