Introducing Python context managers

[thijsmie]

Describe the feature

I've tried out the zenoh-python api and I've noticed the manual opening and closing of sessions and subscriptions. I think it would be way more idiomatic Python to add context managers:

with zenoh.open() as session:
    for key, value in session.info().items():
        print(f"{key}: {value}")

This makes sure the session is automatically closed when exiting the "scope", even when an exception is raised. I have implemented the basic case for session here, but I have to admit that the async case was a bit beyond my current Rust abilities. Implementing these context managers exists of nothing more than implementing __enter__ and __exit__ as methods for the sync case and __aenter__ and __aexit__ for the async case. This would also allow you to make with zenoh.open() return a sync session and async with zenoh.open() return a async session, but that would require an intermediate object with the four methods implemented and would destroy the existing session = zenoh.open() style setup without some potentially ugly trickery.

[p-avital]

Hi,

Edit: this issue will remain open for visibility, but the question is settled

While context managers sound like a good idea, they pose a whole slew of issues in concurrent programs, which we expect most programs depending on Zenoh to be.

This comment elaborates on these issues.

Note that while the Python spec doesn't have true destructors because there is no time-bound on garbage collection, Zenoh Python is only concerned with CPython's behaviour (based on reference counts), since it's the only supported interpreter for it anyway.

Original response

Thanks for the suggestion. We are aware of this feature of Python, and might indeed add support for it to make the code look more pythonic.

However, I think something we don't make clear enough in the documentation is that through the magic of bindings, all zenoh types have actual destructors instead of python's sorry excuse for RAII that with scopes are. This ensures that despite our examples encouraging the user to explicitly destroy their zenoh-values, this is never strictly needed, as the destructors will ensure proper cleanup whenever reference counters reach 0.

This magic is provided by PyO3, and works (I presume) by swapping out the deallocator that python calls whenever refcounts reach zero with the appropriate destructor for the type.

In general, all Zenoh bindings attempt to ensure that you cannot mistakenly forget to properly destroy your objects.

I'm keeping this issue open as a reminder that we might want to support with mostly to reassure pythoners :)

[ipsod]

I agree that switching to context managers would make things a lot more obvious to Python developers.

Also, on something like this, from the subscribe example:

# WARNING, you MUST store the return value in order for the subscription to work!!
# This is because if you don't, the reference counter will reach 0 and the subscription
# will be immediately undeclared.
sub = session.declare_subscriber(key, listener, reliability=Reliability.RELIABLE())

This is surprising behavior in a bad way, I think. It's violation of "Zen of Python": "explicit is better than implicit". It just isn't done. I don't mean to criticize - I'm really impressed with your software. I just think, as far as style goes, that this is pretty matter of fact, from a Python community stand point.

[p-avital]

The behaviour is surprising to most pythoneers, but I do believe the pros of true destructors outweight the cons significantly.

In general, I disagree with Python's "explicit is better than implicit" rule as far as cleanup is concerned, because cleanup is exceedingly easy to forget, and not always as trivial as "this ressource is only needed with this scope". Context managers really only address this by giving you the opportunity to specify that you want cleanup at the same instant in development as when you create a ressource that would need it.

So we end up with 3 cleanup strategies:

• Explicit undeclaration sub.undeclare(): it's just exceedingly easy to forget, and I've seen enough code in languages that don't support destructors to know developpers will forget them.
• Context managers with ... as sub: still rather forgettable, but mostly it just doesn't fit the case of needing to hand over to anything other than local scope, which I expect is somewhat common when sub really is the end of a channel.
• RAII: impossible to forget, able to be moved around, and also allows you to share sub without having to implement your own reference counter around it to know when it's safe to call sub.undeclare().

Keep in mind that, should we make undeclaring subscribers (and other things) explicit only, forgetting to do it isn't just "we're leaking a bit of memory locally" or "we're not closing a file, which may have bad side-effects" level bad; it's "the infrastructure still believes we're interested in that topic and will continue to deliver corresponding data to us until we disconnect, and our callback will keep on being called to process it" level bad.

Finally, we tend to prefer the mindset of "the earlier something breaks, the lesser the impact". With RAII, your subscriber is just gonna be ignored up until you start keeping it around, so your application is just going to not work as long as you don't start using it as described in the example. Unless you never run your code, you won't be able to miss the fact that you're not getting the data you expect. With the others, I bet you'll only figure out that something's wrong once it's actually in production.

[ipsod]

That seems reasonable. Thank you for the explanation.