Response context release

[thehesiod]

don't close connections on exceptions as the library can't make that decision at this point. See discussion in #1568

[fafhrd91]

I don't think this is good solution. What would happen if http message actually malformed? In that case we cannot release connection

[thehesiod]

if it's malformed it should be handled where it's detected that it's malformed. The problem with this is that the exception can be anything, in my case I was just raising Exception from w/in the context causing it to close the connection for no reason.

[fafhrd91]

but when we do release(), connection get returned to pool, and in that case assumption is connection is clean and ready for re-use.

[thehesiod]

can't release() check the transport status? or another way would be to add a custom exception and only close on that exception type. Can you show in the code where we note the transport is corrupt?

[fafhrd91]

transport is just opaque bytes stream. connection can be broken on http level

[thehesiod]

either way the existing code is wrong, how do you know if an exception is not thrown that the transport can be released?

[fafhrd91]

if request parser completes http message parsing then connection is ready for re-use

[thehesiod]

ok how does this sound, if http header parsing does not finish and exception is thrown close connection, otherwise release?

[fafhrd91]

btw what exception do you get? you also can print exc_type

[thehesiod]

here's the issue:

async with http_session.get("http://foobar.com") as response:
   if resp.status != 200:
       raise Exception

this should not cause the connection to be closed as it can be re-used.

[fafhrd91]

ok, that makes sense

[fafhrd91]

here is what you need to do:

if resp.content.at_eof() and resp.content.exception() is None:
     resp.release()
else:
     resp.close()

[fafhrd91]

actually, wait a bit

[fafhrd91]

could you squash your commits?

[fafhrd91]

i modified release method, it should be save to call it even if connection is broken

[thehesiod]

thank you so much for the quick work! I'm sure this has been lurking unnoticed in several people's workflows without them noticing since it "worked" before but would result in things moving slower than they needed to otherwise :)

[fafhrd91]

this may fix one of the issues, but to fix buggy ssl servers with need close timeout.

[thehesiod]

what I'm doing locally is a custom release that forces transfer.abort() instead of .close() if it detects we have an ssl session to the buggy server until that patch is released. I was thinking perhaps aiohttp should enforce the connection limit by waiting until the connection is closed before creating new connections in the "pool". that would prevent socket "leaks" like these in the future but could make things slower if you're constantly closing connections. With the timeout fix you added I think we'll still go slightly over the connector limit while it waits for the close(s) to timeout.

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.