zenoh icon indicating copy to clipboard operation
zenoh copied to clipboard

feat: allow subscriber to be run in background

Open wyfo opened this issue 1 year ago • 13 comments

Background subscribers have their lifetime bound to the session one. They are undeclared when the session is closed, not when they are dropped.

This is a quick feature proposal I've in mind this morning. It's mainly meant to solve "the subscriber api not being pythonic" issue, but it may also be convenient for Rust users too. The principle would of course be extended to Queryable.

The idea for the Python bindings is to use background=true by default, and set background to false when using the context manager form.

@kydos @Mallets @milyin @DenisBiryukov91

wyfo avatar May 06 '24 10:05 wyfo

Actually, I've realized I don't need to set background to false when using context manager, because even if background is true, Subscriber::undeclare can still be called.

wyfo avatar May 14 '24 06:05 wyfo

@wyfo Please change the base branch to dev/1.0.0 as protocol_changes is no longer the development branch.

fuzzypixelz avatar May 17 '24 10:05 fuzzypixelz

As this feature is important for Python semantic I think it should be added. This is an update to Rust API so it have to be additionally decided, do we need it in Rust and other bindings? So at this moment I propose to put it under "unstable" feature and later we can decide, do we need to make it part of API or just put it under "internal" mod to use it for Python only. @Mallets, what do you think?

milyin avatar May 27 '24 08:05 milyin

I've recently realized that I would still have to maintain an internal pool of objects (subscribers/queryables/etc.) in the Python session wrapper because of the order of destruction of Python objects (session would still be destroyed first because declared first, so it would need to drop dependent subscribers/queryables/etc. first, because they still maintain a reference on the session). So I should be able to implement the feature in Zenoh Python without having it in Rust. But as it is a semantic addition to the API, I would prefer to have it in the Rust API too, as I don't think semantic additions in bindings are a good thing. Moreover, it would still be better for the implementation on Python side, as it would avoid to juggle with blocking destructors of dependent objects, only using the session one.

wyfo avatar May 27 '24 08:05 wyfo

I think this doesn't change a lot. This is not a semantic change, this is just addition which is necessary for python and may be useful for other languages, but this is not decided yet. So I don't see any problem to implement it and put to unstable for now

milyin avatar May 27 '24 09:05 milyin

I think you misunderstood. I need an internal pool to allow

with zenoh.open(...) as session:
    sub = session.declare_subscriber(...)

to work, but it does not change the fact that session.declare_subscriber(...) without variable declaration (or context manager) should be allowed to work, because the opposite is completely unpythonic.

wyfo avatar May 27 '24 09:05 wyfo

As I wrote in the issue:

It's mainly meant to solve "the subscriber api not being pythonic" issue, but it may also be convenient for Rust users too.

The pseudo-RAII issue of Python (requiring the internal pool) is a Python specific problem and is orthogonal to the unpythonicity one.

wyfo avatar May 27 '24 09:05 wyfo

Am I correct that this feature is required to write:

with zenoh.open(...) as session:
    session.declare_subscriber(...)

and be sure that subscriber works inside the whole with block, not dropped immediately after declare_subscriber line ?

milyin avatar May 27 '24 09:05 milyin

This feature is not "required", as I can find a way to make it work in my implementation. However, it is required to have a semantic consistency between the APIs.

wyfo avatar May 27 '24 11:05 wyfo

This feature is not "required", as I can find a way to make it work in my implementation. However, it is required to have a semantic consistency between the APIs.

Ok, understand. But statements below are still true, correct?

  • this feature (keeping object alive until session is dropped) is required for python to keep code idiomatic. Specifically, the RAII concept is not so popular in python, so users can easily forget to assign subscriber to variable to keep it alive. And there is no way to ensure that user have to assign return value from "declare_subscriber"
  • it's easier to implement this functionality in Python if it's already provided by Rust. Though it also can be implemented separately

So I think the right way is to provide background entities from Rust as "unstable" and later decide, should they go to the main api or should it be moved to "internal" as binding-specific feature

milyin avatar May 27 '24 12:05 milyin

Specifically, the RAII concept is not so popular in python, so users can easily forget to assign subscriber to variable to keep it alive.

There is no RAII in Python, it's not about popularity. Having a finalizer that may be executed at the end of a function, if there is no exception, and in random order is not RAII, and it's not something reliable. That's why you cannot replace a context manager with a finalizer, and that's why you would expect such behavior of having to declare a variable.

And there is no way to ensure that user have to assign return value from "declare_subscriber"

You don't want to know possible ways of doing it. (It's Python, almost everything is possible 🙂)

it's easier to implement this functionality in Python if it's already provided by Rust. Though it also can be implemented separately

Correct.

wyfo avatar May 27 '24 13:05 wyfo

I think we can go ahead with this feature under an unstable feature gate. Similar approach should be applied to declare_queryable.

Mallets avatar May 28 '24 13:05 Mallets

I think we can go ahead with this feature under an unstable feature gate. Similar approach should be applied to declare_queryable.

Agree. @wyfo please implement it on Rust for all necessary entities under [unstable]

milyin avatar May 28 '24 14:05 milyin

Superseded by #1364

Mallets avatar Sep 11 '24 08:09 Mallets