Codecov Report

Merging #1924 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1924      +/-   ##
==========================================
+ Coverage   97.05%   97.06%   +0.01%     
==========================================
  Files          37       38       +1     
  Lines        7610     7637      +27     
  Branches     1328     1336       +8     
==========================================
+ Hits         7386     7413      +27     
  Misses        101      101              
  Partials      123      123

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c96322c...89accd7. Read the comment docs.

getting older this MR :) any feedback or to much busy these days @asvetlov @fafhrd91

Hmm. The PR adds extra level of complexity to aiohttp client API but be could live with it probably.
_throttle_dns_events doesn't close events on .
If event have an exception inside it will be reported as 'exception was never retrieved'.
Also I strongly object against asyncio.Event hacking. This class is no intended for inheritance. If you need custom implementation -- just copy-paste it and extend on your own. Or use standard locking primitives.

@asvetlov fair enough about the locking primitive, if the Event class is changed in some way it might break the derivated class. I will refactor it avoiding inheritance.

Not sure what you mean with

_throttle_dns_events doesn't close events on

Events are supposed to be temporary to only lock those requests that had a miss in the cache and were keen on the same host resolution. Once the creator of the event either has filled the cache or got an exception, the event is removed from _throttle_dns_events. What Im missing?

Also not sure about:

If event have an exception inside it will be reported as 'exception was never retrieved'.

Regarding the complexity I would like to justify why this change can be interesting.The following code shows a situation where 10 request to the same domain are made when there is a cache miss

import asyncio
import aiohttp

from aiohttp.connector import TCPConnector
from aiohttp.resolver import DefaultResolver

class MyResolver(DefaultResolver):

    def __init__(self, *args, **kwargs):
        self.cnt = 0
        super(MyResolver, self).__init__(*args, **kwargs)

    async def resolve(self, *args, **kwargs):
        self.cnt += 1
        return (await super(MyResolver, self).resolve(*args, **kwargs))



async def fetch(session, url):
    await session.get(url)

async def main():
    url = 'http://www.vilaweb.cat/'
    resolver = MyResolver()
    connector = TCPConnector(resolver=resolver, use_dns_cache=True)
    session = aiohttp.ClientSession(connector=connector)
    tasks = [fetch(session, url) for i in range(10)]
    await asyncio.gather(*tasks)
    print('DNS Resolve calls %d' % resolver.cnt)
    await fetch(session, url)
    print('DNS Resolve calls %d' % resolver.cnt)
    await fetch(session, url)
    print('DNS Resolve calls %d' % resolver.cnt)
    await fetch(session, url)
    print('DNS Resolve calls %d' % resolver.cnt)
    await fetch(session, url)
    print('DNS Resolve calls %d' % resolver.cnt)

if __name__ == '__main__':
    asyncio.get_event_loop().run_until_complete(main())

This is by design. First consecutive calls will be paused at the same code path, and they wont be resumed until the next full scheduler iteration. This MR get rid of that issue having as a result only one DNS query.

Ok, your motivation is very clean.

That I'm talking about is missing getting errors from _throttle_dns_events in TCPConnector.close().

Suppose we have non-processed yet failed events in _throttle_dns_events.
We call

connector.close()
del connector

Pretty simple scenario, isn't it?
Python destroys all connector internal data, and future objects that have stored exceptions inside raises 'exception was never retrieved' warning with more comprehensive info in PYTHONASYNCIODEBUG mode.

To get rid of it you should handle these exceptions in connectior.close()

Checking this scenario I could see that the simple execution of set_exception will create a new reference for the specific Future involved with the loop, because of the call soon [1]. Hence, the destructor of the Future won't be called. IMHO this is the behavior that I will expect, the same for the happy path set_result. Therefore, the explicit close of the TCPconnector should not raise this log entry in execution time.

True that once the program finishes this messages might be printed if the exception is not gathered by the caller, but IMHO this is or it should be the default behavior.

[1] https://github.com/python/cpython/blob/master/Lib/asyncio/futures.py#L129

No. Default should be finishing without warning.
Just call fut.cancel() or fut.exception()

Handle the futures that might be already finished with exception but are still waiting to be processed by the callers will require some deep changes in that MR, have in mind that the current pattern pop the keys as soon as the first caller ends [1]. Main reason, once the cache is filled does not make sense keep stacking future callers in the Event.

Aside of that, I'm still not convinced about that need. Stop the TCPconnector explicitly and make a del of the instance won't always delete the instance, only if there are no more references to them.

Correct me if Im wrong, but even without this MR the current code can suffer the same issue. Let me explain.

Imagine that you have a bunch of callers that are paused waiting for the DNS response, and these have finished with an error, indeed the error is an exception saved to the futures that link the paused callers. If the TCPConnector is stoped and deleted - in fact wont be deleted - the callers will still have the chance to gather that exception if the loop keeps on motion, or if the loop is stoped the exception will be printed to the log.

If this is true, fix the issue that you claim shouldnt make sense because its the same behaviour that right now is happening in the current code.

[1] https://github.com/aio-libs/aiohttp/pull/1924/files#diff-7f25afde79f309e2f8722c26cf1f10adR699

@asvetlov I've modified the Event class getting rid of the inheritance, basically doing a full copy of the official class and trying to get the 100% of coverage.

Also, Ive left the throtle_dns enabled by default, it does not make sense have a flag to enable it or disable it. The user won't enable it because in most of the scenarios won't be aware of the problematic.

Regarding the controversial topic of the futures. I would like to have another reviewer and gather other opinions. what do you think?

Sorry, I'm almost not available until roughly end of month.
Please wait for a while for review.

any ideas @fafhrd91 why the make doc-spelling is failing in Travis?

it works on my computer

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.