-
Notifications
You must be signed in to change notification settings - Fork 173
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
Close builder #1576
Close builder #1576
Conversation
yellowhatter
commented
Oct 31, 2024
•
edited
Loading
edited
- Close builder added with implementations for Session and Runtime
- Remove ephemeral ports in tests
PR missing one of the required labels: {'bug', 'documentation', 'dependencies', 'new feature', 'enhancement', 'internal', 'breaking-change'} |
- add timeout for async task controller finalization
fix ephemeral ports in tests
- remove backoff from CloseBuilder - simplify some code around CloseBuilder
Conflicts: zenoh/src/api/session.rs
Ci failed because of this: #1586 for |
Hi @yellowhatter , here are my comments.
|
Hi, @YuanYuYuan
This is intentional. They used internal future timeout, and now I rely on top-level future timeout that can be modified by the user. |
Without entering the internal details of this particular PR, I think this kind of API addition should be coordinated with @milyin for all the bindings (C, C++, Python, etc.). Adding a timeout in the close builder/option is something we may want to expose as well in all other languages. |
Sure. This change doesn't break the current bindings, and we will coordinate on it's support everywhere after this one get merged |
Yes. We could first discuss the use cases to see what options we want to expose. For instance, making the zenoh session terminate itself in a manner of nonblocking way if it has already been exit stage. |
for this: ZettaScaleLabs/rmw_zenoh#35 |
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.
Looks good, but test for new functionality is needed
Conflicts: zenoh/src/api/session.rs
|
added tests |