cheroot icon indicating copy to clipboard operation
cheroot copied to clipboard

Fixed circular reference in HTTPConnection

Open francis-clairicia opened this issue 3 months ago β€’ 6 comments

❓ 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

This change is Reviewable

francis-clairicia avatar Sep 17 '25 12:09 francis-clairicia

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     

codecov[bot] avatar Sep 17 '25 12:09 codecov[bot]

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 avatar Oct 03 '25 16:10 francis-clairicia

@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 avatar Oct 03 '25 19:10 webknjaz

@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 avatar Oct 07 '25 11:10 francis-clairicia

@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.

webknjaz avatar Oct 15 '25 01:10 webknjaz

@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.

webknjaz avatar Dec 02 '25 16:12 webknjaz