Skip to content

Nolocal close #434

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

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

Conversation

yellowhatter
Copy link
Contributor

No description provided.

@yellowhatter yellowhatter added enhancement New feature or request release Part of the next release api sync Synchronize API with other bindings labels Mar 12, 2025
@yellowhatter yellowhatter self-assigned this Mar 12, 2025
@yellowhatter yellowhatter marked this pull request as ready for review March 26, 2025 08:56
@yellowhatter yellowhatter marked this pull request as draft March 26, 2025 08:57
@yellowhatter yellowhatter marked this pull request as ready for review March 26, 2025 09:49
@@ -0,0 +1,38 @@

//
// Copyright (c) 2024 ZettaScale Technology
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be 2025 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/// A function to receive close handle. If set, the close operation will be executed concurrently
/// in separate task, and this function will receive a handle to be used for controlling
/// close execution.
std::function<void(CloseHandle&&)> out_concurrent = nullptr;
Copy link
Contributor

@DenisBiryukov91 DenisBiryukov91 Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply return close handle from close ? Or maybe even better make a separate close_async. Btw why can't we just only pass a timeout and move session close call to a separate async C++ task and just wait on its promise ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd need an overload by return value to do this. And this is not available in C++ :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is still a common version of close that doesn't return handle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close can always return handle, just in case of missing timeout it can be NULL, or as I suggested make a separate close_async. Also what is wrong with using C++ async task with blocking close and timeout ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout is not tied to async close, it is ok to use it even with blocking close.

The pros to use "our" async close instead of making std::async:

  • "our" close internals in Rust are covered with tests to work in atexit etc
  • "our" close allows dropping a session once close handle is created and even before the actual close completed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, common sense tells that async does not need a timeout (because its non-blocking), so timeout should logically be an option only for a sync call (otherwise it does not make sense at all - is it amount of time that is provided to session to close itself ? then what happens if it does not manage to do it ? - it is not at all clear from the docs).

We can totally keep the behavior of rust, I just suggest to wrap handle into a promise over a simple function that waits on it to avoid creating unnecessary data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of close_async, but thus we need also to fix zenoh-c to have similar behavior. I will make this after the release and rmw code freee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api sync Synchronize API with other bindings enhancement New feature or request release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants