Ensure writer is always reset on completion #7815
Conversation
|
Not sure why these changes triggered some type errors to suddenly appear, but have just fixed them here anyway. |
psf-chronographer
bot
added
the
bot:chronographer:provided
Codecov Report
@@ Coverage Diff @@
## master #7815 +/- ##
==========================================
- Coverage 97.41% 97.40% -0.01%
==========================================
Files 106 106
Lines 32115 32134 +19
Branches 3728 3735 +7
==========================================
+ Hits 31285 31301 +16
- Misses 627 630 +3
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
|
The assert doesn't hold, as I thought the callback would be run immediately upon task completion, but it actually gets scheduled with loop.call_soon() resulting in a race condition. I don't think this actually causes much of a drawback though, we can still rely on the callback to reset the attribute, and we don't seem to need any .done() checks or manually resetting the attribute. |
Backport to 3.9: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 8f2f048 on top of patchback/backports/3.9/8f2f048ed7b0a01630ba620c521f12c673a006e3/pr-7815 Backporting merged PR #7815 into master
🤖 @patchback |
(cherry picked from commit 8f2f048)
|
Everything has been running ok with this PR since the latest revision |
| @@ -178,7 +184,7 @@ class ClientRequest: | |||
| auth = None | |||
| response = None | |||
|
|
|||
| _writer = None # async task for streaming data | |||
| __writer = None # async task for streaming data | |||
There was a problem hiding this comment.
@Dreamsorcerer FYI using double leading underscored is usually discouraged due to how it's re-exposed in the inherited objects...
There was a problem hiding this comment.
It was on purpose, if someone messes with this in an inherited class, they may cause the program to hang or similar.
There was a problem hiding this comment.
@Dreamsorcerer but why do you want it to be exposed for messing it up in the first place?
There was a problem hiding this comment.
I'm not sure I understand what you mean? I'm trying to discourage anyone from accessing/setting this attribute directly.
There was a problem hiding this comment.
@Dreamsorcerer yeah, that's a common mistake and is not the primary use case of the name mangling. It's actually discouraged to use leading double underscores because of the side effects this may cause for people who aren't supposed to know the base class tree implementation details. A single underscore is preferred.
Double leading underscore is advertised to be a hack for dealing with name clashes in the context of inheritance. The end-users would need to know about this implementation detail and never call their classes and attributes with the same name as one of the indirect base classes up the chain.
One CPython Core Dev once told me that the name mangling mechanism is a half-baked band-aid.
I couldn't find any clearly documented dangers of using this, so I had to draft my own example. Here you go:
Python 3.12.0rc3 (main, Sep 22 2023, 15:37:03) [GCC 12.3.1 20230526] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class SomeBase:
... def __init__(self): self.__private_thing = 'Framework Super Base'
... def do_stuff(self): print(f'The ultimate base: {self.__private_thing=}')
...
>>> class AFrameworkProvidedSomeClass(SomeBase): pass
...
>>> # <<<<< BOUNDARY BETWEEN THE FRAMEWORK AND THE END-USER APP >>>>>
>>>
>>> class SomeBase(AFrameworkProvidedSomeClass): # End-user project defining their internal base for reuse
... def __init__(self):
... super().__init__()
... self.__private_thing = 'End-user App Base'
... def show_that_private_thing(self): print(f'Our attr: {self.__private_thing=}')
...
>>> class BaseForMyApp(SomeBase): pass
...
>>> class MyAppFinal(BaseForMyApp):
... def do_our_thing(self): print(f'{dir(self)=}')
...
>>>
>>>
>>> # <<<<< THE END-USER APP JUST WORKS WITH THE CLASS ON THEIR SIDE, NOT KNOWING THE FRAMEWORK IMPLEMENTATION DETAILS >>>>>
>>>
>>>
>>> app = MyAppFinal()
>>> app.do_stuff() # This private API should be predictable because, ...right? Nope!
The ultimate base: self.__private_thing='End-user App Base'
>>> app.show_that_private_thing()
Our attr: self.__private_thing='End-user App Base'
>>> app._SomeBase__private_thing
'End-user App Base'
>>> app.do_our_thing()
dir(self)=['_SomeBase__private_thing', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getstate__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', 'do_our_thing', 'do_stuff', 'show_that_private_thing']
>>> MyAppFinal.__mro__
(<class '__main__.MyAppFinal'>, <class '__main__.BaseForMyApp'>, <class '__main__.SomeBase'>, <class '__main__.AFrameworkProvidedSomeClass'>, <class '__main__.SomeBase'>, <class 'object'>)
>>>
>>>
>>> # What if we call it differently?
>>> class AnotherBase(AFrameworkProvidedSomeClass): # End-user project defining another internal base for reuse
... def __init__(self):
... super().__init__()
... self.__private_thing = 'App Base'
... def show_that_private_thing(self): print(f'Our attr: {self.__private_thing=}')
...
>>> class BaseForMyApp(AnotherBase): pass
...
>>> class MyAppFinal(BaseForMyApp):
... def do_our_thing(self): print(f'{dir(self)=}')
...
>>> app = MyAppFinal()
>>> app.do_stuff() # This private API happens to be predictable because, ...the end-user was lucky not use use the same names. By accident.
The ultimate base: self.__private_thing='Framework Super Base'
>>> app.show_that_private_thing()
Our attr: self.__private_thing='App Base'
>>> app._SomeBase__private_thing
'Framework Super Base'
>>> app.do_our_thing()
dir(self)=['_AnotherBase__private_thing', '_SomeBase__private_thing', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getstate__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', 'do_our_thing', 'do_stuff', 'show_that_private_thing']
>>> MyAppFinal.__mro__
(<class '__main__.MyAppFinal'>, <class '__main__.BaseForMyApp'>, <class '__main__.AnotherBase'>, <class '__main__.AFrameworkProvidedSomeClass'>, <class '__main__.SomeBase'>, <class 'object'>)In this example, the end-user unknowingly adds a class that happens to have the same base name as something from a framework. And decides to use a "__private" attribute that they would "own".
This shouldn't influence anything, right? Well, no. They try to use the framework's public API and it "works", except that their own "private" attribute leaked into the namespace where the framework's attribute with the same name is defined, effectively shadowing it. This is a straight way to break the framework guarantees, never realizing it. And their editor helpfully doesn't auto-complete the super-base private attribute (also because it's actually exposed as _SomeBase__private_thing at the time it's evaluated).
There was a problem hiding this comment.
Oh, and another one:
Python 3.12.0rc3 (main, Sep 22 2023, 15:37:03) [GCC 12.3.1 20230526] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class SomeBase:
... def __init__(self): self.__private_thing = 'Framework Super Base'
... def do_stuff(self): print(f'The ultimate base: {self.__private_thing=}')
...
>>> class AFrameworkProvidedSomeClass(SomeBase): pass
...
>>> class AnotherBase(AFrameworkProvidedSomeClass):
... def access_app_stuff(self): print(f'The private app thing: {self.__private_thing=}')
...
>>> class SomeBase(AnotherBase):
... def access_app_stuff(self): print(f'The private app thing: {self.__private_thing=}')
...
>>> AnotherBase().do_stuff()
The ultimate base: self.__private_thing='Framework Super Base'
>>> SomeBase().do_stuff()
The ultimate base: self.__private_thing='Framework Super Base'
>>> AnotherBase().access_app_stuff()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 2, in access_app_stuff
AttributeError: 'AnotherBase' object has no attribute '_AnotherBase__private_thing'. Did you mean: '_SomeBase__private_thing'?
>>> SomeBase().access_app_stuff()
The private app thing: self.__private_thing='Framework Super Base'
>>>In #7815 the writer was wrapped with a setter to make sure it was always reset on completion and maintain compat for subclasses. This added a tiny bit of overhead since we access self._writer in a lot of places. We can access self.__writer instead since its all inside the class which has less overhead.
No description provided.