Ensure aiosonic only close the connection from one place #483
Conversation
Issue sonic182#473 is still present, just not very often like it used to be. PR sonic182#474 was a first try at getting rid of every RuntimeError possible. It alleviated the problem but it seems it is a smarter choice to only really try to forcefully end the connection from Connection._connect() and only when needed, i.e. a close to begin a new connection.
|
@sonic182 although all tests are passing, later on I'm getting ConnectionPoolAcquireTimeout. Granted, my Timeout for ConnectionPoolAcquire is set at only 1 second, but this does not look auspicious and hints at some logic still missing in this fixup. I therefore advise against merging it as-is. |
|
@sonic182 to make matters worse I think there's a performance penalty in this fix. When we closed the connection early, it was a screw-up I admit, aiosonic would somehow manage to wait on the close twice and the result was somekind of immortal waiter immune for example to setting the reader and writer to None, and this in turn caused the RuntimeError's But when we close earlier there is a performance gain. I haven't measured how much slower it is with the fix, but my ConnectionPoolAcquireTimeout must be set higher, say 10 seconds, instead of 1 second which previously worked fine and faster (but with RuntimeError's) |
The work started at sonic182#474 aimed to get rid of every RuntimeError. Now that is understood that we must close the Connection from one place only in the code to prevent RuntimeError, it appears as superficial to actually wait on the transport to close and to make sure a read into the void is done to any trailing buffer. Doing none of those things still results in functional aiosonic without RuntimeError's and allow us to go back to sync code for Connection release and close, thus allowing to relax the need of pass an instance the Running Loop to HttpResponse.__init__()
|
Performance seems to be restored now that we abort the Connection Transport and forget about it - from one place only :) |
According to performance.py it shows an increase of 10% in speed in relation to aiohttp, consistently. Stats relative to my machine of course, but the improvement is notable.
|
Thanks @sonic182 ! |
Issue #473 is still present, just not very often like it used to be. PR #474 was a first try at getting rid of every RuntimeError possible. It alleviated the problem but it seems it is a smarter choice to only really try to forcefully end the connection from Connection._connect() and only when needed, i.e. a close to begin a new connection.