4 ms·

Frankenstein's `init`

99 points by todsacerdoti 2y ago

smitty1e 2y ago
To paraphrase Wheeler/Lampson: "All problems in python can be solved by another level of indiscretion."
- jerf 2y ago
  Oh, that's good. That has some depth to it. I'm going to have to remember that one. Of course one can switch out "Python" for whatever the local situation is too.
_Algernon_ 2y ago
fix: add time.sleep(0.1) in the .close method.
- gnfargbl 2y ago
  # NOTE: doubled this to 0.2s because of some weird test failures.
- Const-me 2y ago
  Using sleep instead of proper thread synchronization is unreliable. Some people have slow computers like a cheap tablet or free tier cloud VM. Other people have insufficient memory for the software they run and their OS swaps. When I need something like that, in my destructor I usually ask worker thread to shut down voluntarily (using a special posted message, manual reset event, or an atomic flag), then wait for the worker thread to quit within a reasonable timeout. If it didn’t quit within the timeout, I log a warning message if I can.
  - btown 2y ago
    Notably on this point, resource contention on e.g. a Kubernetes cluster can cause these kinds of things to fail spuriously. Sleeps have their uses, but assuming that work is done while you’re sleeping is rarely a good idea, because when it rains it will pour.
  - crdrost 2y ago
    I believe _Algernon_ was making a joke about how you could take this problem and make the code even worse, not proposing a solution to the problem. It's also not even a problem with slow computers or insufficient memory, __init__ does I/O here, it connects to ZeroMQ, so it could have arbitrary latency in various circumstances exceeding the 100 milliseconds that we would be sleeping for. So the joke is, this fixes the problem in your test environment where everything is squeaky clean and you know that ZeroMQ is reachable, and now you have bugs in prod still.
deleted 2y ago
[deleted]
zokier 2y ago
Pretty simple to fix by changing the _init to something like: def _init(self): init_complete = threading.Event() def worker_thread_start(): FooWidget.__init__(self) init_complete.set() self.run() worker_thread = Thread(target=worker_thread_start, daemon=True) worker_thread.start() init_complete.wait() Spawning worker thread from constructor is not that crazy, but you want to make sure the constructing is completed by the time you return from the constructor.
- ohr 2y ago
  Thanks, I hate it! Jk, but would you consider this a good solution overall?
  - gpderetta 2y ago
    I would consider this an hack. But a reasonable hack if you can't change the base class.
  - im3w1l 2y ago
    Not him, but I would also consider making FooBarWidget have a FooWidget rather than be a FooWidget. Normally with a FooWidget you can create one on some thread and then perform operations on it on that thread. But in the case of the FooBarWidget you can not do operations on it because operations must be done in the special thread that is inaccessible.
  - shiandow 2y ago
    I can't come up with a single good reason why you would wish to use inheritance in this case. Except possibly for type checking, but 1. Python has duck typing anyway 2. It's debatable whether these two classes should be interchangeable 3. You shouldn't need to use inheritance just to make the type checking work. It could be considered a clever hack in some situations, but it's completely unsurprising that it causes issues. Putting band-aids on it after you find a bug does not fix the real problem.
- greatgib 2y ago
  To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. It would have been better to an addition start, run, exec method that does that kind of things. Even if for usage it is an inch more complicated to use with an additional call.
draw_down 2y ago
[dead]
wodenokoto 2y ago
> solving an annoying problem with a complete and utter disregard to sanity, common sense, and the feelings of other human beings. While I enjoy comments like these (and the article overall!), they stand stronger if followed by an example of a solution that regards sanity, common sense and the feelings of others.
- crdrost 2y ago
  So in this case that would be, a FooBarWidget is not a subclass of FooWidget but maybe still a subclass of AbstractWidget above that. It contains a thread and config as its state variables. That thread instantiates a FooWidget with the saved config, and runs it, and finally closes its queue. The problem still occurs, because you have to define what it means to close a FooBarWidget and I don't think python Thread has a “throw an exception into this thread” method. Just setting the should_exit property, has the same problem as the post! The thread might still be initing the object and any attempt to tweak across threads could sometimes tweak before init is complete because init does I/O. But once you are there, this is just a tweak of the FooWidget code. FooWidget could respond to a lock, a semaphore, a queue of requests, any number of things, to be told to shut down. In fact, Python has a nice built-in module called asyncio, which implements tasks, and tasks can be canceled and other such things like that, probably you just wanted to move the foowidget code into a task. (See @jackdied's great talk “Stop Writing Classes” for more on this general approach. It's not about asyncio specifically, rather it's just about how the moment we start throwing classes into our code, we start to think about things that are not just solving the problem in front of us, and solving the problem in front of us could be done with just simple structures from the standard library.)
mjr00 2y ago
Doing anything like this in __init__ is crazy. Even `Path("config.json").read_text()` in a constructor isn't a good idea. Friends don't let friends build complicated constructors that can fail; this is a huge violation of the Principle of Least Astonishment. If you require external resources like a zeromq socket, use connect/open/close/etc methods (and a contextmanager, probably). If you need configuration, create a separate function that parses the configuration, then returns the object. I appreciate the author's circumstances may have not allowed them to make these changes, but it'd drive me nuts leaving this as-is.
- abdusco 2y ago
  My go-to is a factory classmethod: class Foo: def __init__(self, config: Config): ... @classmethod def from_config_file(cls, filename: str): config = # parse config file return cls(config)
- echelon 2y ago
  Not just Python. Most languages with constructors behave badly if setup fails: C++ (especially), Java, JavaScript. Complicated constructors are a nuisance and a danger. Rust is the language I'm familiar with that does this exceptionally well (although I'm sure there are others). It's strictly because there are no constructors. Constructors are not special language constructs, and any method can function in that way. So you pay attention to the function signature just like everywhere else: return Result<Self, T> explicitly, heed async/await, etc. A constructor is no different than a static helper method in typical other languages. new Thing() with fragility is vastly inferior to new_thing() or Thing::static_method_constructor() without the submarine defects. Enforced tree-style inheritance is also weird after experiencing a traits-based OO system where types don't have to fit onto a tree. You're free to pick behaviors at will. Multi-inheritance was a hack that wanted desperately to deliver what traits do, but it just made things more complicated and tightly coupled. I think that's what people hate most about "OOP", not the fact that data and methods are coupled. It's the weird shoehorning onto this inexplicable hierarchy requirement. I hope more languages in the future abandon constructors and strict tree-style and/or multi-inheritance. It's something existing languages could bolt on as well. Loose coupling with the same behavior as ordinary functions is so much easier to reason about. These things are so dated now and are best left in the 60's and 70's from whence they came.
pjot 2y ago
Rather than juggling your parent’s __init__ on another thread, it’s usually clearer to: 1. Keep all of your object–initialization in the main thread (i.e. call super().__init__() synchronously). 2. Defer any ZMQ socket creation that you actually use in the background thread into the thread itself.
- ledauphin 2y ago
  yeah, this just feels like one of those things that's begging for a lazy (on first use) initialization. if you can't share or transfer the socket between threads in the first place, then your code will definitionally not be planning to use the object in the main thread.
devrandoom 2y ago
That's Python's constructor, innit?

Frankenstein's `__init__`

99 points by todsacerdoti 2y ago

smitty1e 2y ago

To paraphrase Wheeler/Lampson: "All problems in python can be solved by another level of indiscretion."

jerf 2y ago
Oh, that's good. That has some depth to it. I'm going to have to remember that one. Of course one can switch out "Python" for whatever the local situation is too.

_Algernon_ 2y ago

fix: add time.sleep(0.1) in the .close method.

gnfargbl 2y ago
# NOTE: doubled this to 0.2s because of some weird test failures.
Const-me 2y ago
Using sleep instead of proper thread synchronization is unreliable. Some people have slow computers like a cheap tablet or free tier cloud VM. Other people have insufficient memory for the software they run and their OS swaps. When I need something like that, in my destructor I usually ask worker thread to shut down voluntarily (using a special posted message, manual reset event, or an atomic flag), then wait for the worker thread to quit within a reasonable timeout. If it didn’t quit within the timeout, I log a warning message if I can.
- btown 2y ago
  Notably on this point, resource contention on e.g. a Kubernetes cluster can cause these kinds of things to fail spuriously. Sleeps have their uses, but assuming that work is done while you’re sleeping is rarely a good idea, because when it rains it will pour.
- crdrost 2y ago
  I believe _Algernon_ was making a joke about how you could take this problem and make the code even worse, not proposing a solution to the problem. It's also not even a problem with slow computers or insufficient memory, __init__ does I/O here, it connects to ZeroMQ, so it could have arbitrary latency in various circumstances exceeding the 100 milliseconds that we would be sleeping for. So the joke is, this fixes the problem in your test environment where everything is squeaky clean and you know that ZeroMQ is reachable, and now you have bugs in prod still.

deleted 2y ago

[deleted]

zokier 2y ago

Pretty simple to fix by changing the _init to something like: def _init(self): init_complete = threading.Event() def worker_thread_start(): FooWidget.__init__(self) init_complete.set() self.run() worker_thread = Thread(target=worker_thread_start, daemon=True) worker_thread.start() init_complete.wait() Spawning worker thread from constructor is not that crazy, but you want to make sure the constructing is completed by the time you return from the constructor.

ohr 2y ago
Thanks, I hate it! Jk, but would you consider this a good solution overall?
- gpderetta 2y ago
  I would consider this an hack. But a reasonable hack if you can't change the base class.
- im3w1l 2y ago
  Not him, but I would also consider making FooBarWidget have a FooWidget rather than be a FooWidget. Normally with a FooWidget you can create one on some thread and then perform operations on it on that thread. But in the case of the FooBarWidget you can not do operations on it because operations must be done in the special thread that is inaccessible.
- shiandow 2y ago
  I can't come up with a single good reason why you would wish to use inheritance in this case. Except possibly for type checking, but 1. Python has duck typing anyway 2. It's debatable whether these two classes should be interchangeable 3. You shouldn't need to use inheritance just to make the type checking work. It could be considered a clever hack in some situations, but it's completely unsurprising that it causes issues. Putting band-aids on it after you find a bug does not fix the real problem.
greatgib 2y ago
To be clear, a good init is not supposed to create a thread or do any execution that might not be instant of things like that. It would have been better to an addition start, run, exec method that does that kind of things. Even if for usage it is an inch more complicated to use with an additional call.

draw_down 2y ago

[dead]

wodenokoto 2y ago

> solving an annoying problem with a complete and utter disregard to sanity, common sense, and the feelings of other human beings. While I enjoy comments like these (and the article overall!), they stand stronger if followed by an example of a solution that regards sanity, common sense and the feelings of others.

crdrost 2y ago
So in this case that would be, a FooBarWidget is not a subclass of FooWidget but maybe still a subclass of AbstractWidget above that. It contains a thread and config as its state variables. That thread instantiates a FooWidget with the saved config, and runs it, and finally closes its queue. The problem still occurs, because you have to define what it means to close a FooBarWidget and I don't think python Thread has a “throw an exception into this thread” method. Just setting the should_exit property, has the same problem as the post! The thread might still be initing the object and any attempt to tweak across threads could sometimes tweak before init is complete because init does I/O. But once you are there, this is just a tweak of the FooWidget code. FooWidget could respond to a lock, a semaphore, a queue of requests, any number of things, to be told to shut down. In fact, Python has a nice built-in module called asyncio, which implements tasks, and tasks can be canceled and other such things like that, probably you just wanted to move the foowidget code into a task. (See @jackdied's great talk “Stop Writing Classes” for more on this general approach. It's not about asyncio specifically, rather it's just about how the moment we start throwing classes into our code, we start to think about things that are not just solving the problem in front of us, and solving the problem in front of us could be done with just simple structures from the standard library.)

mjr00 2y ago

Doing anything like this in __init__ is crazy. Even `Path("config.json").read_text()` in a constructor isn't a good idea. Friends don't let friends build complicated constructors that can fail; this is a huge violation of the Principle of Least Astonishment. If you require external resources like a zeromq socket, use connect/open/close/etc methods (and a contextmanager, probably). If you need configuration, create a separate function that parses the configuration, then returns the object. I appreciate the author's circumstances may have not allowed them to make these changes, but it'd drive me nuts leaving this as-is.

abdusco 2y ago
My go-to is a factory classmethod: class Foo: def __init__(self, config: Config): ... @classmethod def from_config_file(cls, filename: str): config = # parse config file return cls(config)
echelon 2y ago
Not just Python. Most languages with constructors behave badly if setup fails: C++ (especially), Java, JavaScript. Complicated constructors are a nuisance and a danger. Rust is the language I'm familiar with that does this exceptionally well (although I'm sure there are others). It's strictly because there are no constructors. Constructors are not special language constructs, and any method can function in that way. So you pay attention to the function signature just like everywhere else: return Result<Self, T> explicitly, heed async/await, etc. A constructor is no different than a static helper method in typical other languages. new Thing() with fragility is vastly inferior to new_thing() or Thing::static_method_constructor() without the submarine defects. Enforced tree-style inheritance is also weird after experiencing a traits-based OO system where types don't have to fit onto a tree. You're free to pick behaviors at will. Multi-inheritance was a hack that wanted desperately to deliver what traits do, but it just made things more complicated and tightly coupled. I think that's what people hate most about "OOP", not the fact that data and methods are coupled. It's the weird shoehorning onto this inexplicable hierarchy requirement. I hope more languages in the future abandon constructors and strict tree-style and/or multi-inheritance. It's something existing languages could bolt on as well. Loose coupling with the same behavior as ordinary functions is so much easier to reason about. These things are so dated now and are best left in the 60's and 70's from whence they came.

pjot 2y ago

Rather than juggling your parent’s __init__ on another thread, it’s usually clearer to: 1. Keep all of your object–initialization in the main thread (i.e. call super().__init__() synchronously). 2. Defer any ZMQ socket creation that you actually use in the background thread into the thread itself.

ledauphin 2y ago
yeah, this just feels like one of those things that's begging for a lazy (on first use) initialization. if you can't share or transfer the socket between threads in the first place, then your code will definitionally not be planning to use the object in the main thread.

devrandoom 2y ago

That's Python's constructor, innit?