Skip to content

[Backport]Fix subscription.is_serialized() for callbacks with message info (#1950)#2622

Merged
fujitatomoya merged 1 commit intoros2:humblefrom
roscan-tech:bp/humble/pr-1950
Oct 7, 2024
Merged

[Backport]Fix subscription.is_serialized() for callbacks with message info (#1950)#2622
fujitatomoya merged 1 commit intoros2:humblefrom
roscan-tech:bp/humble/pr-1950

Conversation

@roscan-tech
Copy link
Copy Markdown

  • Fix subscription.is_serialized() for callbacks with message info argument

  • Add tests + please linters

…2#1950)

* Fix subscription.is_serialized() for callbacks with message info argument

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Add tests + please linters

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: roscan-tech <liwei4402@mail2.sysu.edu.cn>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

I think we can backport this.

@roscan-tech it seems that you are trying to cherry-pick some fixes from the main to humble. Do you have any specific scope or requirement for doing that?

@roscan-tech
Copy link
Copy Markdown
Author

roscan-tech commented Sep 10, 2024

@fujitatomoya Thank you for your attention to my pull requests!

Actually, we are working on a research project aimd at improving sotware reliability for ROS2 systems. In particular, we are exploring a new, automatic approach to identify cross-version(distribution/branch) discripencies related to bug-fix.

As we know, ROS2 has multiple distributions, but security fixes are not always applied consistently across all of them. What we are doing is to automatically identify these bugs and vulnerabilities to ensure that the fixes are synchronized across different versions.

While Mergify is an excellent tool for automating merge processes, it still requires manual judgment and activation. Therefore, it might be beneficial to develop an automated bot that can assess whether a fix needs to be synchronized across different versions.

The potential bugs we currently raised are partically the output of an automated tool, with manual verificaiton. We sincerely appricate your effors helping us confiming these issuse.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@roscan-tech thank you for sharing that.

Actually, we are working on a research project aimd at improving sotware reliability for ROS2 systems. In particular, we are exploring a new, automatic approach to identify cross-version(distribution/branch) discripencies related to bug-fix.

The reason that you picked ROS 2 is that you guys use ROS 2 (probably humble) for robot application? Or main purpose is to test your automation tool with ROS 2?

fixes are not always applied consistently across all of them.

this is true, some fixes are missing even it is ABI compatible. (this PR is an example.)

@clalancette
Copy link
Copy Markdown
Contributor

As we know, ROS2 has multiple distributions, but security fixes are not always applied consistently across all of them. What we are doing is to automatically identify these bugs and vulnerabilities to ensure that the fixes are synchronized across different versions.

So the problem with what we've seen so far as that none of these are security fixes (or, at least, they are not clearly security fixes). Further, just having automation backport things is causing a bunch of work on us, and for no clear benefit. Yes, there are probably bugs in the older distributions, but we have a very small team working on this. Putting additional load on that group to review these automated backports actually makes life worse for our users, because we can't prioritize the things that actually matter (i.e. bugs that real users are running into).

Thus, my opinion on this is that we should not do this kind of automated backporting. This is not the last word, and I want to hear opinions from other people on @ros2/team, but I'd appreciate it if you paused opening more of these until the ROS PMC makes a decision regarding these.

@roscan-tech
Copy link
Copy Markdown
Author

roscan-tech commented Sep 11, 2024

The reason that you picked ROS 2 is that you guys use ROS 2 (probably humble) for robot application? Or main purpose is to test your automation tool with ROS 2?

@fujitatomoya Thank you for your attention to and feedback on our work.
In fact, we identified synchronization issues across different versions within ROS 2 and designed a tool to address this. We have tested our tool on the humble, iron, rolling, and foxy distributions and the foxy version had the most cross-version issues.

@roscan-tech
Copy link
Copy Markdown
Author

roscan-tech commented Sep 11, 2024

@clalancette @fujitatomoya
Thank you for your attention to and feedback on our work. We sincerely apologize for any inconvenience and time we may have caused. We will promptly cease further PR submissions.

Our primary objective is to enhance the reliability and consistency of ROS 2 distributions by identifying and synchronizing fixes across different versions. While the fixes we have submitted may not strictly fall under the category of critical security patches, we believe they significantly contribute to system stability and user experience. Our intention is not to impose unnecessary burdens but to assist in making ROS 2 more robust and secure. We are committed to refining our methods and tools to focus on identifying and submitting only the most critical fixes. We would greatly appreciate collaboration from the community in developing such tools together and believe that automating the process of identifying and synchronizing fixes will ultimately benefit the community by ensuring that essential patches are not overlooked in any distribution.

If a dedicated team is already addressing this issue, our automation tool could be valuable. While it may add some short-term burden, an effective automated tool can help reduce the workload of the team in the long term. Given that ROS 2 is relatively new and evolving rapidly, the likelihood of synchronization bugs is likely to increase over time. Therefore, such automation tools seem necessary for sustainable development.

We will pause further submissions until the ROS PMC reaches a decision. Meanwhile, we are keen to continue the discussion and find a balanced approach that aligns with the community’s needs.

Most of the fixes I submitted target commits previously labeled as bugs, which might have led to some misunderstandings. However, I still believe that these fixes warrant backporting. For example, the bug described in ros2/rclpy@ad22613 appears to be quite significant and remains unresolved in the foxy version. There may be other critical bugs that I have not yet had the opportunity to examine due to time constraints.

Thank you again for your valuable input and patience. We look forward to continuing the dialogue with the community to find a balanced solution to these issues.

@wjwwood wjwwood requested a review from fujitatomoya October 3, 2024 13:14
@fujitatomoya
Copy link
Copy Markdown
Collaborator

@roscan-tech we have had discussion in PMC meeting, and we are going to try ABI compatibility checker in rpr job. we believe that is better and consistent process to manage the backports to the downstream distribution than picking up some fixes like this. besides, based on Interim policy on the use of generative AI in OSRF projects we need to refrain from using generative AI tools or bot. So no further PRs generated by AI could be taken. for backports, we will ask the reason and background to the developer and contributor, assess critical severity.
we will review ros2/launch_ros#411 and #2622, since those are created before announcement and content of those PRs are already covered and acceptable.

Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

this is the backport of #1950, lgtm with green CI

@fujitatomoya
Copy link
Copy Markdown
Collaborator

Pulls: #2622
Gist: https://gist.githubusercontent.com/fujitatomoya/c557ed897df99e56237f2f32f759b0e1/raw/cc504a694912d880f15bf8879db36011edc1a872/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14666

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Oct 7, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator

RHEL failures are unrelated https://ci.ros2.org/job/ci_linux-rhel/1473/cmake/new/ (RTI cmake) and https://ci.ros2.org/job/ci_linux-rhel/1473/testReport/ (python yaml). i will go ahead to merge this.

@fujitatomoya fujitatomoya merged commit 28de27e into ros2:humble Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants