-
Notifications
You must be signed in to change notification settings - Fork 913
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
Possible solution to #577 by reworking onRetryTimer() logic #660
Possible solution to #577 by reworking onRetryTimer() logic #660
Conversation
Can one of the admins verify this patch? |
@dirk-thomas could I get a pull request builder run? Thanks! |
Test failed. |
Ok, it appears only one error is present, and it's |
The Jenkins changelog just shows the other PR since it has been merged since the job was run last time. I will trigger another run to see if the test is just flaky. |
Test failed. |
I don't think the failing test is The test executable invoked is https://github.com/ros/ros_comm/blob/2faea451097966190c4d6e5a989f0228e53c8bc8/utilities/message_filters/test/test_subscriber.cpp |
Well that's is pretty strange. The test builds & runs fine on my workstation against this PR:
|
Test failed. |
I looked further into the Jenkins error message and it seems that it is simply not able to extract the correct standard output since multiple tests run concurrently and therefore the console output is mixed. The top of the stacktrace is accurate though:
Based on that I looked into the full console output and searched for
|
Quick update, now that I am looking at the right test in my local workspace, things are a bit more obvious:
That recursive mutex error doesn't seem to show up on Jenkins. I figured if there were any errors in this patch, it would be a deadlock condition, so it's just going to a matter of uncovering the deadlock. |
The full console output does contain the exact error message: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/ARCH_PARAM=amd64,UBUNTU_PARAM=trusty,label=devel/374/consoleFull |
That it does! Additionally, there are 9 instances of the string 'recursive_mutex.hpp' in that full console output, all of which point to this mutex failure, though not all of them cause test errors. That doesn't bode well for this patch. |
@dirk-thomas Could I snag a pull request builder run? @jeffadamsc and I worked through the logic surrounding |
Test passed. |
Superseded by #670 |
This is an alternative to #654 by attempting to lock out the
dropping_
flag. I'll explain it in more detail if it proves to be useful.