Fixed circular reference in HTTPConnection
β What kind of change does this PR introduce?
- [x] π bug fix
- [ ] π£ feature
- [ ] π docs update
- [ ] π tests/coverage improvement
- [ ] π refactoring
- [ ] π₯ other
π What is the related issue number (starting with #)
Resolves #746
β What is the current behavior? (You can also link to an open issue here)
The UNIX socket server's caching system for credentials involves a circular reference to HTTPConnection, even for an INET server.
β What is the new behavior (if this is a feature change)?
The cache system is still in place, but now uses private attributes.
π Other information:
π Contribution checklist:
(If you're a first-timer, check out this guide on making great pull requests)
- [x] I wrote descriptive pull request text above
- [x] I think the code is well written
- [x] I wrote good commit messages
- [ ] I have squashed related commits together after the changes have been approved
- [ ] Unit tests for the changes exist
- [x] Integration tests for the changes exist (if applicable)
- [x] I used the same coding conventions as the rest of the project
- [x] The new code doesn't generate linter offenses
- [x] Documentation reflects the changes
- [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 83.99%. Comparing base (ac9b6e5) to head (ff66b04).
:white_check_mark: All tests successful. No failed tests found.
Additional details and impacted files
@@ Coverage Diff @@
## main #755 +/- ##
==========================================
- Coverage 84.01% 83.99% -0.03%
==========================================
Files 28 28
Lines 4235 4242 +7
==========================================
+ Hits 3558 3563 +5
- Misses 677 679 +2
I'm not sure to understand: where is the complexity?
A decorator-based implementation will result in the same issue than functools.lru_cache and there will be much more complexity in the decorator implementation in order not to create a circular defence than by simply have a private attribute.
I really don't see the issue here.
@francis-clairicia would using a @cached_property work? This would mean having a new API and we could keep the old methods uncached for compat.
@webknjaz This option seems good. The only drawback is that a @cached_property is writable, whereas @property is read-only by default.
If that's OK with you, then I can make the changes accordingly.
@francis-clairicia I didn't realize it was writable. But here's how we can make it read-only. Put this helper into a _functools.py, so that it lives in a dedicated space. Add a test/_functools_test.py to test the setter raising an error so that it all's got good test coverage.
>>> import functools as _ft
>>> class readonly_cached_property(_ft.cached_property):
... def __set__(self, instance, value):
... attr_name = self.attrname
... raise AttributeError(f'Attribute {attr_name} is read-only and cannot be set.')
>>> class AttrsCls:
... @cached_property
... def cp(self):
... return 42
... @readonly_cached_property
... def rocp(self):
... return 84
>>> attro = AttrsCls()
>>> attro.rocp
84
>>> attro.cp
42
>>> attro.rocp = 3
Traceback (most recent call last):
File "<python-input-42>", line 1, in <module>
attro.rocp = 3
^^^^^^^^^^
File "<python-input-37>", line 4, in __set__
raise AttributeError(f'Attribute {attr_name} is read-only and cannot be set.')
AttributeError: Attribute rocp is read-only and cannot be set.
Additionally, make the change note a bit more specific. Referencing an abstract unnamed circular ref does not help a reader understand the context and the effect this has on them.
@francis-clairicia hey, should I expect you to get back to addressing the review request in this PR? It's okay if not β I'll take over eventually, just not sure about the timeline.