Hacker News Reader

Comment by zokier | Hacker News Reader

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.

View on HN

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.

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

That breaks RAII - you shouldn't give the user a way to create the object in an invalid state. Although if it's intended to be used as a context manager anyway then maybe doing it on context enter rather than on init would be nicer.

Doesn't necessarily have to be in an invalid state. You can have your object with a none value for the socket or a state like "not_started" and the different method will look at that to handle everything properly.

Also, it was not supposed to be used in a context manager as there is just a close method but it is the caller that decided to wrap it in a context.

But as what you suggest, indeed it would have been a great idea to add an __enter__ and do the starting there are as it would be the proper way.

I don't think it's totally crazy to, say, open a DB connection in __init__() even though that's not an instantaneous operation. That's not a hill I would die on, you can just say “open a connection and hand it to me as an argument,” but it just looks a little cleaner to me if the lifecycle of the connection is being handled inside this DB-operations class. (You can also defer creating the connection until it is actually used, or require an explicit call to connect it, but then you are also writing a bunch of boilerplate to make the type checker happy for cases where the class is having its methods called before it was properly connected.)

It's not totally crazy in that I see it all the time, but it's one of the two most common things I've found make Python code difficult to reason about.[0] After all, if you open a DB connection in __init__() -- how do you close it? This isn't C++ where we can tie that to a destructor. I've run into so many Python codebases that do this and have tons of unclosed connections as a result.

A much cleaner way (IMO) to do this is use context managers that have explicit lifecycles, so something like this:

    @contextmanager
    def create_db_client(host: str, port: int) -> Generator[_DbClient, None, None]:
        try:
            connection = mydblib.connect(host, port)
            client = _DbClient(connection)
            yield client
        finally:
            connection.close()


    class _DbClient:
        def __init__(self, connection):
           self._connection = connection
        
        def do_thing_that_requires_connection(...):
           ...

Which lets you write client code that looks like

    with create_db_client('localhost', 5432) as db_client:  # port 3306 if you're a degenerate
        db_client.do_thing_that_requires_connection(...)

This gives you type safety, connection safety, has minimal boilerplate for client code, and ensures the connection is created and disposed of properly. Obviously in larger codebases there's some more nuances, and you might want to implement a `typing.Protocol` for `_DbClient` that lets you pass it around, but IMO the general idea is much better than initializing a connection to a DB, ZeroMQ socket, gRPC client, etc in __init__.

[0] The second is performing "heavy", potentially failing operations outside of functions and classes, which can cause failures when importing modules.

I get the points everyone is making and they make sense, but sometimes you need persistent connections. Open and closing constantly like can cause issues

There's nothing about the contextmanager approach that says you're open and closing any more or less frequently than a __init__ approach with a separate `close()` method. You're just statically ensuring 1) the close method gets called, and 2) database operations can only happen on an open connection. (or, notably, a connection that we expect to be open, as something external the system may have closed it in the meantime.)

Besides, persistent connections are a bit orthogonal since you should be using a connection pool in practice, which most Python DB libs provide out of the box. In either case, the semantics are the same, open/close becomes lease from/return to pool.

> This isn't C++ where we can tie that to a destructor

`def __del__`

> def __del__

Nope, do not ever do this, it will not do what you want. You have no idea _when_ it will be called. It can get called at shutdown where the entire runtime environment is in the process of being torn down, meaning that nothing actually works anymore.

C++ destructors are deterministic. Relying on a nondeterministic GC call to run __del__ is not good code.

Also worth noting that the Python spec does not say __del__ must be called, only that it may be called after all references are deleted. So, no, you can't tie it to __del__.

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

Maybe it indirectly ensures that only one thread is created per instantiation; there are better ways of achieving that though.

What if the object is in an invalid state unless the thread is started?

Nothing prevents you to have a valid "not_yet_started" state.

No, but it does complicate things substantially. I don't understand why you would have a useless half-constructed state, especially in python where init isn't all that magical.

The benefits of making illegal states unrepresentable are just not widely understood outside of pure FP circles. I think it's hard for stuff like this to catch on in python in particular because everything is so accessible and mutable, it would be hard to talk about anything that provides the level of confidence that eg. ADTs in Haskell provide. And it's hard to understand why you would want to try to do something like that in a context like python unless you already have experience with it in a statically typed context.

This is basically equivalent to what null is for pointers, and has all the same problems.

And what would you do with this object after construction? Call start() on it immediately?

Kind of yes or no, depend on your usage. But clearly would have worked well for the case of the automatic test if the start was the context

Thanks, I hate it! Jk, but would you consider this a good solution overall?

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.

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.

I would consider this an hack. But a reasonable hack if you can't change the base class.

Reply on news.ycombinator.com