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

Nolocal close #1632

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

yellowhatter
Copy link
Contributor

Implement close in the way it can be safely waited and awaited in atexit

@yellowhatter yellowhatter self-assigned this Dec 5, 2024
@yellowhatter yellowhatter added enhancement Existing things could work better release Part of the next release labels Dec 5, 2024
@yellowhatter yellowhatter marked this pull request as ready for review December 5, 2024 13:36
@yellowhatter yellowhatter requested a review from milyin December 5, 2024 13:36
@evshary
Copy link
Contributor

evshary commented Dec 10, 2024

It still failed when I tested with the following code

use std::sync::OnceLock;
use zenoh::{Config, Session, Wait};
static SESSION: OnceLock<Session> = OnceLock::new();

fn main() {
    let s = SESSION.get_or_init(|| zenoh::open(Config::default()).wait().unwrap());
    println!("> zid: {}", s.zid());
    unsafe { libc::atexit(close_session) };
}

extern "C" fn close_session() {
    SESSION.get().unwrap().close().wait().unwrap();
}

@yellowhatter
Copy link
Contributor Author

yellowhatter commented Dec 18, 2024

It still failed when I tested with the following code

use std::sync::OnceLock;
use zenoh::{Config, Session, Wait};
static SESSION: OnceLock<Session> = OnceLock::new();

fn main() {
    let s = SESSION.get_or_init(|| zenoh::open(Config::default()).wait().unwrap());
    println!("> zid: {}", s.zid());
    unsafe { libc::atexit(close_session) };
}

extern "C" fn close_session() {
    SESSION.get().unwrap().close().wait().unwrap();
}

Tokio (and some other nolocal-unsafe libs) does not allow interacting with it's objects from atexit handling thread. However, already existing threads (including those dedicated to the async runtime executor) are ok to do that, even during concurrent atexit execution.

The current Close implementation in this PR does not intend Session::close to be called inside atexit, it just allows waiting inside atexit for prior started nonblocking close to complete - that's why you get panic. This was enough to fix rmw errors in scenario I tested - but now I see we need more comprehensive thing.

It is possible to make close to be both callable and waitable inside atexit. The logic will be: upon Session creation a close task is spawned. This task asynchronously awaits for nolocal-safe async primitive (based on async_channel) to be triggered. This task will stay inactive for the whole Session lifetime. Once the primitive is triggered (from elsewhere, even from atexit), the task will proceed and execute asynchronous internal close routine.
In other words, this is a reverse approach on how NolocalJoinHandle is implemented in this PR.

@Mallets @evshary I will implement the described approach in this PR next

@YuanYuYuan
Copy link
Contributor

YuanYuYuan commented Dec 23, 2024

Hi @yellowhatter, the RP seems not to work with the following code.

use std::sync::OnceLock;
use zenoh::{Config, Session, Wait};
static SESSION: OnceLock<Session> = OnceLock::new();

fn main() {
    let s = SESSION.get_or_init(|| zenoh::open(Config::default()).wait().unwrap());
    println!("> zid: {}", s.zid());
    unsafe { libc::atexit(close_session) };
}

extern "C" fn close_session() {
    SESSION.get().unwrap().close().in_background().wait();
}

Can you briefly state what this PR aims to resolve?

Regarding your nolocal close idea, how do you avoid the underlying Zenoh runtime spawning a new task on a new thread for termination and then block_in_place on it? This may still happen.

@yellowhatter
Copy link
Contributor Author

Hi @yellowhatter, the RP seems not to work with the following code.

use std::sync::OnceLock;
use zenoh::{Config, Session, Wait};
static SESSION: OnceLock<Session> = OnceLock::new();

fn main() {
    let s = SESSION.get_or_init(|| zenoh::open(Config::default()).wait().unwrap());
    println!("> zid: {}", s.zid());
    unsafe { libc::atexit(close_session) };
}

extern "C" fn close_session() {
    SESSION.get().unwrap().close().in_background().wait();
}

Can you briefly state what this PR aims to resolve?

This PR just allows you to wait() for Session close to complete inside atexit handler. However, it still doesn't allow close() to be called from atexit because after some discussion with Luca we decided not to spend time on this functionality and put it to the very long-term backlog.

Regarding your nolocal close idea, how do you avoid the underlying Zenoh runtime spawning a new task on a new thread for termination and then block_in_place on it? This may still happen.

I didn't get the problem here, can you please explain it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants