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.

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

start thread only after successfully adding the connection #603

Merged
merged 2 commits into from
Apr 16, 2015
Merged

start thread only after successfully adding the connection #603

merged 2 commits into from
Apr 16, 2015

Conversation

dirk-thomas
Copy link
Member

Aims to address #544

@dirk-thomas dirk-thomas mentioned this pull request Apr 13, 2015
Closed
@ros-pull-request-builder
Copy link
Member

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/222/

@tfoote
Copy link
Member

tfoote commented Apr 16, 2015

This looks reasonable. If we merge this with the symptom workaround with a good error message pointing back to the ticket it should be safe.

@rethink-rlinsalata @dirk-thomas
fix add_connection leak on already closed SubImpl
362c57f 
Protect against race condition where a new `TCPROSTransport`
connection is created and added to a `_SubscriberImpl` that is
already closed. Checks and returns False if closed so transport
creator can shutdown socket connection. Fixes issue #544.
@dirk-thomas dirk-thomas mentioned this pull request Apr 16, 2015
Closed
@dirk-thomas
Copy link
Member Author

The first commit tries to fix the actual problem so that a connection should never be added to a closed subscriber in the first place.

The second commit will still catch the case and prevent a memory leak. If that still happens the error message points the user to the ticket to provide feedback.

@esteve @tfoote @wjwwood Please review.

@tfoote
Copy link
Member

tfoote commented Apr 16, 2015

+1

@esteve
Copy link
Member

esteve commented Apr 16, 2015

LGTM

dirk-thomas added a commit that referenced this pull request Apr 16, 2015
@dirk-thomas
Merge pull request #603 from ros/start_thread_only_after_adding_conne…
a88e77b 
…ction

start thread only after successfully adding the connection
@dirk-thomas dirk-thomas merged commit a88e77b into indigo-devel Apr 16, 2015
@dirk-thomas dirk-thomas deleted the start_thread_only_after_adding_connection branch April 16, 2015 19:14
@ros-pull-request-builder
Copy link
Member

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/228/

@mpatalberta
Copy link

can you please direct me to where in indigo ros the fix has been made for
@dirk-thomas dirk-thomas referenced this pull request on Apr 13, 2015
Closed
rospy: Connection / Memory Leak, adding conn. to closed _SubscriberImpl #544

@dirk-thomas
Copy link
Member Author

@mpatalberta Please see the line above:

dirk-thomas merged commit a88e77b into indigo-devel on Apr 16, 2015

The commit mentions version 1.11.11 as the first release the patch has been part of.

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017
@dirk-thomas
Merge pull request ros#603 from ros/start_thread_only_after_adding_co…
9e80a1e 
…nnection

start thread only after successfully adding the connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dirk-thomas @ros-pull-request-builder @tfoote @esteve @mpatalberta @rethink-rlinsalata