Skip to content

Make web.BaseRequest and web.StreamResponse weak referenceable#4375

Merged
asvetlov merged 3 commits intoaio-libs:masterfrom
Fogapod:weakref
Nov 25, 2019
Merged

Make web.BaseRequest and web.StreamResponse weak referenceable#4375
asvetlov merged 3 commits intoaio-libs:masterfrom
Fogapod:weakref

Conversation

@Fogapod
Copy link
Copy Markdown
Contributor

@Fogapod Fogapod commented Nov 23, 2019

What do these changes do?

Adding __weakref__ to __slots__ makes web.BaseRequest and web.StreamResponse valid targets for creating weak references.

Are there changes in behavior for the user?

Users are able to create weak references to request and response objects again after #3942.

Related issue number

Resolves #4368
Resolves #4370

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 23, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 23, 2019

Codecov Report

Merging #4375 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4375   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files          43       43           
  Lines        8861     8861           
  Branches     1389     1389           
=======================================
  Hits         8641     8641           
  Misses         96       96           
  Partials      124      124
Impacted Files Coverage Δ
aiohttp/web_response.py 97.48% <ø> (ø) ⬆️
aiohttp/web_request.py 97.57% <ø> (ø) ⬆️

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 e70ac6d...e6a5be4. Read the comment docs.

Copy link
Copy Markdown
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fogapod your RP is 100% correct, thank you!
Would you add a couple of tests that create a weak reference for both web.Request and web.Response objects?
I want never to make this mistake again.

Thanks!

@Fogapod
Copy link
Copy Markdown
Contributor Author

Fogapod commented Nov 23, 2019

@asvetlov could you draft a new alpha release after this is merged? This bug causes issues on production for me, installing aiohttp from source is not easy

@untitaker
Copy link
Copy Markdown

@Fogapod I mean, you're running an alpha version of aiohttp, right?

@Fogapod
Copy link
Copy Markdown
Contributor Author

Fogapod commented Nov 25, 2019

I'm currently on 4.0.0a1, yes

@Fogapod Fogapod requested a review from asvetlov November 25, 2019 16:06
@asvetlov asvetlov merged commit dd85639 into aio-libs:master Nov 25, 2019
@asvetlov
Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

web.Request and web.Response are no longer weak referenceable WebSocketResponse TypeError: Cannot be put into a weakref.WeakSet

4 participants