Skip to content

🧑‍🌾 Windows test_executors regression: SEH Exception #2749

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
Open
Crola1702 opened this issue Feb 18, 2025 · 4 comments · May be fixed by #2767
Open

🧑‍🌾 Windows test_executors regression: SEH Exception #2749

Crola1702 opened this issue Feb 18, 2025 · 4 comments · May be fixed by #2767
Assignees
@jmachowinski
Labels
help wanted Extra attention is needed

Comments

@Crola1702
Copy link
Contributor

Description

This is a consistent regression in windows release and windows repeated

Reference build:

Test regressions:

Log output:

Running main() from C:\ci\ws\install\src\gtest_vendor\src\gtest_main.cc
[==========] Running 28 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 28 tests from TestExecutor
[ RUN      ] TestExecutor.add_remove_node_thread_safe
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
Stack trace:


[  FAILED  ] TestExecutor.add_remove_node_thread_safe (33 ms)
@Crola1702 Crola1702 mentioned this issue Feb 18, 2025
Closed
@jmachowinski
Copy link
Collaborator

When did this start ?

@jmachowinski
Copy link
Collaborator

I just had a look at this, I stumbled over this a few days back. The test is kind of broken as it deletes the node before the executor stopped using it. I would temporarily disable the test, until we agreed on a patch for rclcpp::Executor::remove_node().

@fujitatomoya fujitatomoya pinned this issue Feb 25, 2025
@fujitatomoya fujitatomoya unpinned this issue Feb 25, 2025
@fujitatomoya
Copy link
Collaborator

i think that is okay to disable the test for now.

originally #1505 just adds the thread safety locks for the executor.
if that is so, how about changing the test to make sure that destroys nodes after executor stopped? and we can probably keep the current test (deleting the nodes before executor stops) as comment and enable once the rclcpp::Executor::remove_node() is fixed

@alsora alsora added the help wanted Extra attention is needed label Mar 6, 2025
@alsora
Copy link
Collaborator

alsora commented Mar 6, 2025

We discussed this issue during the waffle meeting and we think that it would be good to disable the test temporarily.
However it's important that we follow-up with work to fix the root cause.

@Crola1702 do you know when this bug started and which changes could have caused it?

@jmachowinski jmachowinski linked a pull request Mar 14, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jmachowinski jmachowinski

Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Fixed possible memory violation after remove_node cellumation/rclcpp
4 participants
@jmachowinski @alsora @fujitatomoya @Crola1702