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

[rospy] made get_published_topics threadsafe #958

Merged
merged 2 commits into from
Jan 10, 2017

Conversation

jabbenseth
Copy link
Contributor

With multiple threads trying to get the currently published topics at least one of them crashes with

Exception in thread Thread-4:
Traceback (most recent call last):
    topics = rospy.get_published_topics()
  File "/ros_comm/clients/rospy/src/rospy/client.py", line 376, in get_published_topics
    code, msg, val = get_master().getPublishedTopics(namespace)
  File "/ros_comm/clients/rospy/src/rospy/msproxy.py", line 106, in wrappedF
    return f(*args, **kwds)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1233, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1587, in __request
    verbose=self.__verbose
  File "/usr/lib/python2.7/xmlrpclib.py", line 1273, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1303, in single_request
    response = h.getresponse(buffering=True)
  File "/usr/lib/python2.7/httplib.py", line 1077, in getresponse
    raise ResponseNotReady()
ResponseNotReady

Exception in thread Thread-5:
Traceback (most recent call last):

    topics = rospy.get_published_topics()
  File "/ros_comm/clients/rospy/src/rospy/client.py", line 376, in get_published_topics
    code, msg, val = get_master().getPublishedTopics(namespace)
  File "/ros_comm/clients/rospy/src/rospy/msproxy.py", line 106, in wrappedF
    return f(*args, **kwds)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1233, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1587, in __request
    verbose=self.__verbose
  File "/usr/lib/python2.7/xmlrpclib.py", line 1273, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1298, in single_request
    self.send_request(h, handler, request_body)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1400, in send_request
    connection.putrequest("POST", handler, skip_accept_encoding=True)
  File "/usr/lib/python2.7/httplib.py", line 906, in putrequest
    raise CannotSendRequest()
CannotSendRequest

the acquisition of f is thread safe, but the final call to f seems not.

@jspricke
Copy link
Member

HI @ipa-jba thanks for the pull request, do you have a simple example to play around?
Looking good at first sight, @dirk-thomas any comments, as you did the implementation?

@jabbenseth
Copy link
Contributor Author

jabbenseth commented Jan 10, 2017

on my machine this script crashes (without modifications) ten out of ten times:

#!/usr/bin/env python

import rospy

rospy.init_node("test_threadsafeness")

for i in range(2):
    rospy.Timer(rospy.Duration(1), rospy.get_published_topics)

rospy.spin()

@dirk-thomas
Copy link
Member

I can reproduce the problem as well as confirm that the patch makes this case pass. Since the proxy class claims that all methods are thread-safe (

All methods are thread-safe.
) it makes sense to lock for the actual call.

To avoid the need for two separate locks the operation to get the function from target could be delayed until the actual call which then could be done together in a single lock:

with self._lock:
    f = getattr(self.target, key)
    return f(*args, **kwds)

@jabbenseth
Copy link
Contributor Author

I updated the code to implement your suggestions @dirk-thomas. One critical information for your review: I tested it on indigo and did an "untested" port to kinetic for this

@dirk-thomas
Copy link
Member

I tested it in Kinetic and CI also runs for Kinetic on this PR. I will wait with the merge until the new CI build passes. Thank you for the patch!

@dirk-thomas dirk-thomas merged commit 5c8997e into ros:kinetic-devel Jan 10, 2017
ggallagher01 pushed a commit to clearpathrobotics/ros_comm that referenced this pull request Jan 26, 2017
* [rospy] made get_published_topics threadsafe
dirk-thomas pushed a commit that referenced this pull request Mar 2, 2017
* [rospy] made get_published_topics threadsafe
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.

3 participants