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.

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

Python Simple Action Server Deadlock Test #8

Closed
wants to merge 4 commits into from

Conversation

miguelsdc
Copy link
Contributor

As discussed in #4, this pull request contains a Python unit test to check for deadlocks during simple_action_server.py's execution. It fails with groovy-devel and hydro-devel, but passes with #6 (which is the expected behaviour).

The patch is composed of three files (plus an extra line in CMakeLists.txt)

  • test_simple_action_server_deadlock.py which implements the main test using python.unittest. The script: sets up a simple action server; services requests from companions; kills the companion nodes; and checks the last serviced request wasn't too long ago.
  • simple_action_server_deadlock_companion.py which simply requests many concurrent TestActions.
  • test_simple_action_server_deadlock_python.launch which launches five companions and the test.

I've identified the following potential issues with this patch:

  • Testing for deadlocks is inherently difficult: a proper solution would require overriding python's RLock(). Instead these scripts allow the deadlock to happen and then checks whether the actions are being carried out. The disadvantage is that, if deadlocked, this test will terminate only when the test timeouts and RLTestTimeoutException() is raised (after ~75 seconds)
  • Even when simple action server doesn't deadlock this test takes about 45 seconds to complete. This is because of a deadlock is not a deterministic condition, and thus we must allow sufficient time for it to happen.
  • test_simple_action_server_deadlock.py imports rosnode which I believe is an implicit dependency of rospy. I don't know if this dependency should be made explicit, leave as it, or removed (it is not vital for the test, it just makes the output cleaner).

I couldn't come up with a better solution, but suggestions to improve this patch are welcome :)

@dirk-thomas
Copy link
Member

This unit test is great - especially since it is always very difficult to test these kind of deadlock. The times are absolutely appropriate - other tests in roscpp do the testing in a similar fashion. For test we can have additional dependencies - I created a pull request which lists rosnode explicity so that it is available when test is being executed on the farm.

@dirk-thomas
Copy link
Member

After you have merged miguelsdc#1 into your repo you might want to squash the commits on the branch into a single commit. Than I will go ahead and merge the test as well as the fix.

Thank you for taking the time to create this unit test!

add test depend, remove usage of roslib.load_manifest, pep8
@dirk-thomas
Copy link
Member

Closed in favor of #9.

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.

None yet

2 participants