-
Notifications
You must be signed in to change notification settings - Fork 36
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
yellowhatter
wants to merge
8
commits into
eclipse-zenoh:main
Choose a base branch
from
ZettaScaleLabs:nolocal_close
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Nolocal close #434
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5e32f46
Update api.hxx, close.hxx, and 2 more files...
yellowhatter 33c95cb
Merge commit 'feaf56812b9bbc9e03bee3edd751494528253d10'
yellowhatter 1da68ad
support concurrent close
yellowhatter f4a4431
clang format
yellowhatter b81edba
Merge commit '6f3324692a679af1e35a450d82baf571a2bbc1a6'
yellowhatter e79eeef
Update zenoh-c
yellowhatter 59deff1
Update zenoh-c
yellowhatter be3f5fe
[skip ci] Update year in close.hxx
yellowhatter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
|
||
// | ||
// Copyright (c) 2025 ZettaScale Technology | ||
// | ||
// This program and the accompanying materials are made available under the | ||
// terms of the Eclipse Public License 2.0 which is available at | ||
// http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
// which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
// | ||
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
// | ||
// Contributors: | ||
// ZettaScale Zenoh Team, <zenoh@zettascale.tech> | ||
|
||
#pragma once | ||
|
||
#if defined(ZENOHCXX_ZENOHC) && defined(Z_FEATURE_UNSTABLE_API) | ||
|
||
namespace zenoh { | ||
|
||
/// A close operation handle. | ||
class CloseHandle : public Owned<::zc_owned_concurrent_close_handle_t> { | ||
friend class Session; | ||
CloseHandle(zenoh::detail::null_object_t) : Owned(nullptr){}; | ||
|
||
public: | ||
/// @brief Blocks until corresponding close operation completes. | ||
/// @param err if not null, the result code will be written to this location, otherwise ZException exception will be | ||
/// thrown in case of error. | ||
void wait(ZResult* err = nullptr) { | ||
__ZENOH_RESULT_CHECK(zc_concurrent_close_handle_wait(interop::as_moved_c_ptr(*this)), err, | ||
"Failed to wait for close operation"); | ||
} | ||
}; | ||
|
||
} // namespace zenoh | ||
|
||
#endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// | ||
// Copyright (c) 2022 ZettaScale Technology | ||
// | ||
// This program and the accompanying materials are made available under the | ||
// terms of the Eclipse Public License 2.0 which is available at | ||
// http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
// which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
// | ||
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
// | ||
// Contributors: | ||
// ZettaScale Zenoh Team, <zenoh@zettascale.tech> | ||
// | ||
|
||
#include "zenoh.hxx" | ||
|
||
using namespace zenoh; | ||
|
||
#undef NDEBUG | ||
#include <assert.h> | ||
|
||
void test_session_close_in_drop() { auto session = Session::open(Config::create_default()); } | ||
|
||
void test_session_close() { | ||
auto session = Session::open(Config::create_default()); | ||
session.close(); | ||
} | ||
|
||
#if defined(ZENOHCXX_ZENOHC) && defined(Z_FEATURE_UNSTABLE_API) | ||
void test_session_close_in_background() { | ||
auto session = Session::open(Config::create_default()); | ||
|
||
bool close_called = false; | ||
auto close_options = Session::SessionCloseOptions::create_default(); | ||
close_options.out_concurrent = [&close_called](CloseHandle&& h) { | ||
h.wait(); | ||
close_called = true; | ||
}; | ||
|
||
session.close(std::move(close_options)); | ||
|
||
if (!close_called) { | ||
exit(-1); | ||
} | ||
} | ||
#endif | ||
|
||
int main() { | ||
test_session_close_in_drop(); | ||
test_session_close(); | ||
#if defined(ZENOHCXX_ZENOHC) && defined(Z_FEATURE_UNSTABLE_API) | ||
test_session_close_in_background(); | ||
#endif | ||
} |
Submodule zenoh-c
updated
4 files
+61 −60 | Cargo.lock | |
+61 −60 | build-resources/opaque-types/Cargo.lock | |
+2 −1 | build-resources/opaque-types/src/lib.rs | |
+4 −6 | src/close.rs |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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++ :)
There was a problem hiding this comment.
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 handleThere was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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