Proposal to change PR "Add __del__ method to BusABC"

[felixdivo]

Proposal to replace #1489. The problem was that if there was an error while initializing the interface, the __del__ would still be triggered and cause weird "unhandled exception" warnings and the cleanup might not complete entirely.

With this change, __del__ only takes action if __init__ succeeded previously. This also allows us to re-enable the warnings that were disabled in e3f2f6e. The downside is that we need to take care where to call super().__init__() in the concrete BusABC implementations.

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #1489. Mergify cannot evaluate rules on this PR. ⚠️

[zariiii9003]

I think __del__ would be very useful for closing opened resources after a failed __init__ method. I played around with the nixnet interface yesterday and this was bothering me a lot. You cannot call shutdown when you do not get a bus instance.

So in my opinion this is the wrong way to deal with this. Disabling warnings is obviously also not great.

The shutdown methods would need to be adapted to handle partially initialized interfaces (e.g. using if hasattr to avoid AttributeErrors).

[Tbruno25]

Agree'd that this is the better approach as opposed to filtering out the warnings 👍

[Tbruno25]

I'm not sure we need 2 separate attributes to implement this. If we only want __del__ to execute if the class is initialized, perhaps _is_active would be more idiomatic?

[felixdivo]

And yes, you are totally right. And if we are adding many hasattr() checks to shutdown() anyways, we can also delete such a marker attribute instead of checking it's value. Not sure if that is a good idea, but it's possible.

[Tbruno25]

That's a great point @zariiii9003

Presumably if __init__ fails there's some sort of error already propagated. Could the correct action be to redirect the output to a debug log if __init__ fails, else raise as normal?

[felixdivo]

I think __del__ would be very useful for closing opened resources after a failed __init__ method.

Thanks @zariiii9003 for looking over this, but I actually disagree with this. __del__ is only a last-resort measure, if the user forgets to implement calling shutdown(), not to help with error handling. This is because we are not guaranteed that it is called, right?

But you are right in that the some of the current interfaces might not gracefully handle exceptions being raised by (functions called in) their __init__. However, we should much rather guard against such failures using try ... finally blocks within the interfaces or using similar techniques, than to use this fallback instead. This is also for helping with exception handling, since if we get a general error like OSError that we actually want to propagate, we should raise our errors instead (raise CanError(...) from original_cause).

[zariiii9003]

Even if you put the whole __init__ content into a try...except clause, you will probably need to call shutdown in the error case. shutdown will still need to be adapted to handle partially initialized bus instances.

[felixdivo]

shutdown will still need to be adapted to handle partially initialized bus instances.

I think there should never be a partially initiliaized interface (from the user's perspective). So either __init__ creates an initialized interface or it fails to do so and leaves a clean state behind.

Well, let's consider a simple example:

def __init__(params, ...):
    self._resource_1 = create_it_but_it_might_raise_an_exception(params)
    self._resource_2 = create_other_but_it_might_raise_an_exception(params)
    super().__init__(...)

My point is that we should make sure that the function never returns in an unsafe state. Because if __init__() raises, we do not have an object that we can call shutdown() on, so in that case self._resource_1 and self._resource_2 should be in a clean state. I do not think that we should rely in __del__ in any case.

So instead, the above code should be written as:

def __init__(params, ...):
    try:
        self._resource_1 = create_it_but_it_might_raise_an_exception(params)
    except OSError as e:
        raise CanInitializationError("resource 1 is not working") from e

    try:
        self._resource_2 = create_other_but_it_might_raise_an_exception(params)
    except OSError as e:
        self._resource_1.close_silently()  # If we have only one socket/... this is not needed and things are quite simple
        raise CanInitializationError("resource 2 is funky") from e
    
    # We only mark the interface as initiallized after the above worked without raising
    super().__init__(...)

[zariiii9003]

I understand your point, but regarding your example: If you don't want to repeat self._resource_1.close_silently(), self._resource_2.close_silently() 20 times in the __init__ method, you put that logic inside the shutdown method. And that shutdown method must be robust against partially initialized instances.

[felixdivo]

Okay, it seems like we can be quite sure that __del__ will be called (https://docs.python.org/3/reference/datamodel.html#object.__del__), if it is not caputured in stack trace information. I'm still not quite convinced that relying on __del__ is a superior architecture, but if you want, feel free to take over this PR and change it as you described. In any case, both approaches will do the job. 👍

[felixdivo]

Are we sure it is called after __init__ raised an exception? Maybe one should try that first.

[zariiii9003]

Are we sure it is called after __init__ raised an exception? Maybe one should try that first.

That's a very good point, it probably won't be called. I'll check that later

[zariiii9003]

I think the simplest fix is to catch the AttributeError

    def __del__(self) -> None:
        if not self._is_shutdown:
            LOG.warning("%s was not properly shut down", self.__class__)
            # We do some best-effort cleanup if the user
            # forgot to properly close the bus instance
            try:
                self.shutdown()
            except AttributeError:
                pass

I also tried the hasattr solution here

[felixdivo]

I think the simplest fix is to catch the AttributeError

Well yeah, but then we need to make sure to release the resources in the order in which they were assigned to the object (for the case that, for example, the exception was thrown somewhere in the middle).

Good thing: This will not silence possible programming errors when calling shutdown() normally. 👍

[felixdivo]

@zariiii9003 Would you like to take over this PR?

[zariiii9003]

@zariiii9003 Would you like to take over this PR?

I think @Tbruno25 should finalize his own PR. I don't think _is_initialized is necessary and _periodic_tasks is a mutable class variable, which isn't great. Maybe @Tbruno25 could cherrypick your other changes though.

[Tbruno25]

Sounds good -- would be happy to wrap this up 👍

[felixdivo]

Then let's continue the discussion in your PR 😄