[3.8] Backport fix for setting cookies

[Dreamsorcerer]

I see this fix in master, but this needs to be released on the 3.7 branch, as aiohttp-session is broken without this.

(cherry-picked from 54afac7)

[asvetlov]

Could you please refer to the master's commit with the fix?

[CLAassistant]

All committers have signed the CLA.

[Dreamsorcerer]

It's this one: 54afac7

Appears to be part of a more substantial change in aiohttp4 (where HTTPException is no longer subclassing Response). This single line I've taken from there is enough to fix the behaviour in v3.7.

[codecov]

Codecov Report

Merging #5233 (ddd00f7) into 3.8 (8ce3013) will increase coverage by 0.01%.
The diff coverage is 94.89%.

@@            Coverage Diff             @@
##              3.8    #5233      +/-   ##
==========================================
+ Coverage   97.53%   97.55%   +0.01%     
==========================================
  Files          44       44              
  Lines        8844     8818      -26     
  Branches     1424     1419       -5     
==========================================
- Hits         8626     8602      -24     
+ Misses        102      101       -1     
+ Partials      116      115       -1     

Flag	Coverage Δ
unit	97.39% <94.89%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/hdrs.py	100.00% <ø> (ø)
aiohttp/http_websocket.py	98.67% <ø> (ø)
aiohttp/locks.py	92.85% <0.00%> (ø)
aiohttp/resolver.py	95.16% <ø> (ø)
aiohttp/web_middlewares.py	100.00% <ø> (ø)
aiohttp/web_routedef.py	98.09% <66.66%> (ø)
aiohttp/streams.py	97.94% <80.00%> (+0.48%)	⬆️
aiohttp/multipart.py	96.97% <85.71%> (ø)
aiohttp/client_reqrep.py	97.20% <87.50%> (-0.02%)	⬇️
aiohttp/__init__.py	100.00% <100.00%> (ø)
... and 43 more

---

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 88f8f3b...eee5eea. Read the comment docs.

[Dreamsorcerer]

That test appears to have passed on the previous commit, so I think it's just flaky (given that I only added a CHANGE file in this commit).

[Dreamsorcerer]

I've added a simplified version of the test in aiohttp-session, which should catch regressions.

[asvetlov]

Cool! I'll take a review soon

[asvetlov]

I'm sorry but the fix is incorrect.
This line was backported by the incident.
Since in aiohttp 3.x HttpException is derived from Response we should just use it as-is without the creation of intermediate response object.
I believe this line was incorrectly backported by #4771

I apologize for the regression.

[Dreamsorcerer]

No problem, I'll just update this with the reverted code then.

Appears to actually have been this commit, which moved some of the logic into a new _handle_request() method:
29eccad#diff-2126b277e07e3fdd05e7a81da456accf24e5515a46c78c48a79db4eb166043e4L373

[Dreamsorcerer]

Apparently, not so easy. Now getting ServerDisconnectedError in the tests.
Will take a look later in the week, unless you beat me to it.

[Dreamsorcerer]

OK, reverting back to using the exception as the response seems to require moving the exception handler back to the parent function in order to handle the deprecation warning correctly. I think that's working now though.

[Dreamsorcerer]

Flaky test? Seems odd that the changes would fail on macos only. Also the CI seems to be struggling, it fails to load the test log half of the time I try to look at it.

[thijstriemstra]

+1! Also ran into this issue and reverted aiohttp to 3.6.3 for now.

[Dreamsorcerer]

Looking back at my previous comments, the current test failure is the same one from earlier, which is just flaky, as it failed on one commit and passed on the next despite only changing documentation.

[asvetlov]

The PR is correct but please move your changes into _handle_request; it reduces the diff between the master and backport branches.

Also, I would freeze 3.7 branch but publish aiohttp 3.8 soon.
Please use 3.8 as the base branch for PR.

[thijstriemstra]

maybe change the pr title as well.

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging eee5eea into 5d1a75e - view on LGTM.com

new alerts:

• 1 for Unused local variable
• 1 for Missing call to __init__ during object initialization

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

[Dreamsorcerer]

First time I've seen a failure from lgtm. :P
Appears to be from the merge though, not related to the PR (in fact, it's stopped failing since the target branch has been changed).

[asvetlov]

Then I see lgtm alerts I usually open "view on LGTM.com" link, analyze it, and close as "I don't care" or "false alarm".
But sometimes it is helpful as a point at "bad smelling" code.

[asvetlov]

Thanks!