Conversation
Codecov Report
@@ Coverage Diff @@
## master #2910 +/- ##
==========================================
+ Coverage 97.98% 97.98% +<.01%
==========================================
Files 40 40
Lines 7506 7511 +5
Branches 1316 1318 +2
==========================================
+ Hits 7355 7360 +5
Misses 48 48
Partials 103 103
Continue to review full report at Codecov.
|
aiohttp/connector.py
Outdated
| req.url.raw_host, | ||
| req.port, | ||
| traces=traces) | ||
| traces=traces)) |
There was a problem hiding this comment.
Please pass loop=self._loop into shield().
Consider it as tiny speed optimization.
aiohttp/connector.py
Outdated
| # Cancelling this lookup should not cancel the underlying lookup | ||
| # or else the cancel event will get broadcast to all the waiters | ||
| # across all connections. | ||
| hosts = await asyncio.shield(self._resolve_host( |
There was a problem hiding this comment.
Is there two spaces between await and asyncio.shield()?
|
Thank you for patch, looks almost good on the first glaze. |
| # Cancelling this lookup should not cancel the underlying lookup | ||
| # or else the cancel event will get broadcast to all the waiters | ||
| # across all connections. | ||
| hosts = await asyncio.shield(self._resolve_host( |
There was a problem hiding this comment.
So this shield [1] can be removed, it was put in place just for the situation that you are commenting but taking a look at your MR - and the shield implementation - I was wrong.
There was a problem hiding this comment.
Ok was wondering about that, removed
|
The change looks good and reasonable but please add CHANGES file. |
|
@asvetlov added |
| @@ -0,0 +1 @@ | |||
| fix cancellation broadcast during DNS resolve | |||
There was a problem hiding this comment.
I'm sorry but the folder name must be upper-cased: CHANGES/2910.bugfix.
The idea is having change-note on the top of changed files list.
There was a problem hiding this comment.
I think this folder was renamed in master, so I ran into this: https://coderwall.com/p/5w3jca/git-case-sensitivity-on-os-x fixed
|
It was uppercased last in 2017, I don't remember the exact date. |
|
LGTM. |
* fix resolve cancellation * fixes based on review * changes based on review * add changes file * rename (cherry picked from commit a7bbaad) Co-authored-by: Alexander Mohr <[email protected]>
* fix resolve cancellation * fixes based on review * changes based on review * add changes file * rename (cherry picked from commit a7bbaad) Co-authored-by: Alexander Mohr <[email protected]>
ghost
mentioned this pull request
|
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. |
psf-chronographer
bot
added
the
bot:chronographer:provided
4 participants
In my situation I have groups of 3 similar requests, where I take the fastest one out of the three and cancel the other two.
Lets say we have two groups of 3: A and B
B.1 succeeds
B.2 blocks on resolve (https://github.com/aio-libs/aiohttp/blob/master/aiohttp/connector.py#L702)
A.1 waits on resolve of B.2 (https://github.com/aio-libs/aiohttp/blob/master/aiohttp/connector.py#L689)
if we cancel B.2, it ends up cancelling A.1 because the exception is broadcast to all waiters: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/locks.py#L31
so if a resolve is cancelled, only the task blocked on the resolve should be cancelled, not all the items waiting on the resolve. Only if it's a DNS error should the error be broadcast.