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.

Sign up for GitHub

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

Jump to bottom

fix domain coordinator API #9

Closed
dirk-thomas opened this issue Aug 28, 2020 · 2 comments · Fixed by #12
Closed

fix domain coordinator API #9

dirk-thomas opened this issue Aug 28, 2020 · 2 comments · Fixed by #12
Assignees
@mabelzhang
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@dirk-thomas
Copy link
Member

Follow up of #3.

At the moment the function get_coordinated_domain_id returns an object wrapping a socket and mentions in the docblock that it relies on the garbage collector to destroy the socket when no references are hold anymore. Since in Python the garbage collector might run anytime (or never) this isn't a viable API.

During the tests even a warning is printed that the sockets are never being closed: https://ci.ros2.org/view/All/job/test_ci_windows/196/pytest-warnings/

@pbaughman Since you create the original pull request implementing this API can you please look into this and revise the API in a follow up PR to make it explicit and deterministic?
@dirk-thomas dirk-thomas added the bug Something isn't working label Aug 28, 2020
This was referenced Aug 28, 2020
Merged
Draft
@pbaughman
Copy link
Contributor

Hi @dirk-thomas. This was part of some work I did when I was contracting for Apex.AI. I'm no longer working with them. I think @dejanpan would be the guy who can help you get this taken care of. He can probably assign someone from the QA team to make the requested updates.

@pbaughman
Copy link
Contributor

pbaughman commented Aug 28, 2020 •
edited
Loading

A note for future implementers - it's probably sufficient to make the domain coordinator a context object and use it in a with statement. That would give this socket/port the desired unambiguous lifetime that dirk is asking for.

@hidmic hidmic added the help wanted Extra attention is needed label Sep 10, 2021
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
b37c791 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9
@roehling roehling mentioned this issue Oct 12, 2021
Merged
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
37f749b 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
533292e 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
1bb8bad 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
ce7758a 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
a504534 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
roehling added a commit to fkie-forks/ament_cmake_ros that referenced this issue Oct 12, 2021
@roehling
Refactor domain_coordinator API to use a context manager
9775a52 
The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes ros2#9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
clalancette pushed a commit that referenced this issue Oct 14, 2021
@roehling
Refactor domain_coordinator API to use a context manager (#12)
b8ee10c 
* Refactor domain_coordinator API to use a context manager

The domain_coordinator refactored to expose a single context manager
which returns a unique ROS_DOMAIN_ID that is valid until the context
goes out of scope.

The function has been deliberately renamed to force fatal errors if
the call site was not updated to use a "with" context.

Fixes #9

Signed-off-by: Timo Röhling <timo.roehling@fkie.fraunhofer.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mabelzhang mabelzhang

Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor domain_coordinator API to use a context manager fkie-forks/ament_cmake_ros
4 participants
@dirk-thomas @pbaughman @mabelzhang @hidmic