Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cut BaseProtocol circular reference on close. #1049

Merged
merged 4 commits into from Oct 9, 2023
Merged

Cut BaseProtocol circular reference on close. #1049

merged 4 commits into from Oct 9, 2023

Conversation

pteromys
Copy link
Contributor

@pteromys pteromys commented Jul 7, 2023

A bound method contains a reference to the instance it's bound to. Most of the time, bound methods are created lazily at access time by the descriptor protocol and discarded after calling. But saving a bound method as another attribute on the instance creates a long-lived cycle, here .timeout_callback.__self__, that needs to be explicitly broken if we don't want to wake up python's garbage collector to do it.

Without this change, the new assertion in the tests would fail, and pytest --pdb would show the bound methods _on_timeout and _on_waiter_completed at the end of p gc.get_referrers(protoref()).

Things I'm not too certain about:

  • whether I got all of the codepaths that need to delete .timeout_callback and .completed_callback—I empirically seem to have covered my use case, but that's far from actually understanding what gets called when
  • whether the test I modified is the right place to put the new assertion
  • windows—I tested on both asyncio and uvloop, but only on linux

elprans
elprans reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

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

TBH, I'm not sure those variables are needed at all, because they always seem to be assigned to self._on_timeout and self._on_waiter_completed, so perhaps just remove them?

@pteromys
Copy link
Contributor Author

pteromys commented Jul 8, 2023

Oh, I like that idea. Done! Let's see if it breaks anything...

pteromys and others added 4 commits October 9, 2023 11:47
@pteromys @elprans
Cut BaseProtocol circular reference on close.
d148b75 
A bound method contains a reference to the instance it's bound to.
Most of the time, bound methods are created lazily at access time by
the descriptor protocol and discarded after calling. But saving a bound
method as another attribute on the instance creates a long-lived cycle,
here `.timeout_callback.__self__`, that needs to be explicitly broken
if we don't want to wake up python's garbage collector to do it.

Without this change, the new assertion in the tests would fail, and
`pytest --pdb` would show the bound methods `_on_timeout` and
`_on_waiter_completed` at the end of `p gc.get_referrers(protoref())`.
@pteromys @elprans
Just remove the callbacks instead.
6a02962
@pteromys @elprans
Fix line length for flake8.
b8e6833
@elprans
Unset transport in Protocol.abort()
ddb4505
@elprans
Copy link
Member

elprans commented Oct 9, 2023

There turned out to be another cycle -- protocol -> transport -> protocol -- which doesn't seem to get broken on the transport side in a Proactor loop on Windows.

@elprans elprans merged commit 93a6f79 into MagicStack:master Oct 9, 2023
33 checks passed
@pteromys
Copy link
Contributor Author

Thanks for following up and fixing that! (Sorry, I should've said something right away when I saw the test failure on Windows and realized it was going to be over my head!)

@pteromys pteromys deleted the callback-gc branch October 10, 2023 16:18
elprans added a commit that referenced this pull request Nov 5, 2023
@elprans
asyncpg v0.29.0
38aba5d 
Minor fixes and improvements.

Improvements
============

* Python 3.12 and PostgreSQL 16 support (#1084)
  (by @elprans in deea86c)

* Add support for tuple-format custom codecs on composite types (#1061)
  (by @elprans in 922fcd1)

* Support `target_session_attrs` in URL format, add tests (#1073)
  (by @elprans in 7cb4e70)

* Infinity numeric support (#1067)
  (by @krokoziabla in 0c3bf60 for #1020)

* Add support for the `WHERE` clause in `copy_to` methods (#941)
  (by @kaylynn234 in b7ffab6)

* Add query logging callbacks and context manager (#1043)
  (by @dcwatson in b2697ff)

Fixes
=====

* When prepared statements are disabled, avoid relying on them harder (#1065)
  (by @elprans in cbf64e1)

* Handle environments with HOME set to a not-a-directory (#1063)
  (by @elprans in af922bc)

* Fix handling of non-ASCII passwords (#1062)
  (by @elprans in 89d5bd0)

* Disable JIT while doing type introspection (#1082)
  (by @elprans in f21ebf6)

* Remove connection parameter caching in `Pool` (#1053)
  (by @ermakov-oleg in 4ddb039)

* Switch to Python 3.12-style `wait_for` (#1086)
  (by @elprans in 4bdd8a7)

* Update automatic PostGIS type conversion for Shapely 2.0 (#1085)
  (by @ChimneySwift in 8b45beb)

* Use the `timeout` context manager in the connection path (#1087)
  (by @elprans in 313b2b2)

* Small fix for documentation on using SSL in Connection (#995)
  (by @ScottFred in ccc7baf)

* Use cleanup_ctx in pool usage doc (#878)
  (by @ir4y in 70c8bd8)

* Close cursor portals once the iterator is exhausted (#1088)
  (by @elprans in ca9f03b)

* Cut BaseProtocol circular reference on close. (#1049)
  (by @pteromys in 93a6f79)

* Allow passing hosts as tuples to `connect()` (in addition to lists) (#1021)
  (by @lezram in d7faaff)

Other
=====

* Drop support for Python 3.7 (#1064)
  (by @bryanforbes in 87ab143)
@elprans elprans mentioned this pull request Nov 5, 2023
Merged
elprans added a commit that referenced this pull request Nov 5, 2023
@elprans
asyncpg v0.29.0
74f3a00 
Minor fixes and improvements.

Improvements
============

* Python 3.12 and PostgreSQL 16 support (#1084)
  (by @elprans in deea86c)

* Add support for tuple-format custom codecs on composite types (#1061)
  (by @elprans in 922fcd1)

* Support `target_session_attrs` in URL format, add tests (#1073)
  (by @elprans in 7cb4e70)

* Infinity numeric support (#1067)
  (by @krokoziabla in 0c3bf60 for #1020)

* Add support for the `WHERE` clause in `copy_to` methods (#941)
  (by @kaylynn234 in b7ffab6)

* Add query logging callbacks and context manager (#1043)
  (by @dcwatson in b2697ff)

Fixes
=====

* When prepared statements are disabled, avoid relying on them harder (#1065)
  (by @elprans in cbf64e1)

* Handle environments with HOME set to a not-a-directory (#1063)
  (by @elprans in af922bc)

* Fix handling of non-ASCII passwords (#1062)
  (by @elprans in 89d5bd0)

* Disable JIT while doing type introspection (#1082)
  (by @elprans in f21ebf6)

* Remove connection parameter caching in `Pool` (#1053)
  (by @ermakov-oleg in 4ddb039)

* Switch to Python 3.12-style `wait_for` (#1086)
  (by @elprans in 4bdd8a7)

* Update automatic PostGIS type conversion for Shapely 2.0 (#1085)
  (by @ChimneySwift in 8b45beb)

* Use the `timeout` context manager in the connection path (#1087)
  (by @elprans in 313b2b2)

* Small fix for documentation on using SSL in Connection (#995)
  (by @ScottFred in ccc7baf)

* Use cleanup_ctx in pool usage doc (#878)
  (by @ir4y in 70c8bd8)

* Close cursor portals once the iterator is exhausted (#1088)
  (by @elprans in ca9f03b)

* Cut BaseProtocol circular reference on close. (#1049)
  (by @pteromys in 93a6f79)

* Allow passing hosts as tuples to `connect()` (in addition to lists) (#1021)
  (by @lezram in d7faaff)

Other
=====

* Drop support for Python 3.7 (#1064)
  (by @bryanforbes in 87ab143)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elprans elprans elprans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pteromys @elprans