Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow subscriber to be run in background #1012

Draft
wants to merge 2 commits into
base: dev/1.0.0
Choose a base branch
from

Conversation

wyfo
Copy link

@wyfo wyfo commented May 6, 2024

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

Background subscribers have their lifetime bound to the session one.
They are undeclared when the session is closed, not when they are dropped.
@wyfo wyfo marked this pull request as draft May 6, 2024 10:33
@wyfo
Copy link
Author

wyfo commented May 14, 2024

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.

@fuzzypixelz
Copy link
Member

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

@wyfo wyfo changed the base branch from protocol_changes to dev/1.0.0 May 17, 2024 10:27
@milyin
Copy link
Contributor

milyin commented May 27, 2024

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?

@wyfo
Copy link
Author

wyfo commented May 27, 2024

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.

@milyin
Copy link
Contributor

milyin commented May 27, 2024

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

@wyfo
Copy link
Author

wyfo commented May 27, 2024

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
Copy link
Author

wyfo commented May 27, 2024

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.

@milyin
Copy link
Contributor

milyin commented May 27, 2024

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 ?

@wyfo
Copy link
Author

wyfo commented May 27, 2024

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.

@milyin
Copy link
Contributor

milyin commented May 27, 2024

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

@wyfo
Copy link
Author

wyfo commented May 27, 2024

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.

@Mallets
Copy link
Member

Mallets commented May 28, 2024

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

@milyin
Copy link
Contributor

milyin commented May 28, 2024

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants