Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Close CLOSE_WAIT sockets (#610)#1086

Closed
otamachan wants to merge 3 commits intoros:lunar-develfrom
otamachan:fix-610
Closed

Close CLOSE_WAIT sockets (#610)#1086
otamachan wants to merge 3 commits intoros:lunar-develfrom
otamachan:fix-610

Conversation

@otamachan
Copy link
Copy Markdown
Contributor

@otamachan otamachan commented Jun 26, 2017

Hi,

I'm having the same issue as in #610 and I found that some of the sockets in caches of ServerProxy(_proxies) are remained CLOSE_WAIT when a slave(xmlrpc server) close its port suddenly.
I added a function to close all the CLOSE_WAIT sockets in caches(f06bdf1), made it an option to call the function in xmlrpcapi (d3e7921) and added --close-sockets option to roscore(00d8f73).

I think calling the close_half_closed_sockets periodically is enough, but I couldn't find the right way so I call it in xmlrpcapi.
I'm happy if closing CLOSE_WAIT sockets is enabled by default, could you review this and sugget a better way?

Thanks in advance.

otamachan added 3 commits June 27, 2017 05:38
@otamachan
Add close_half_closed_sockets function
f06bdf1
@otamachan
Add enable_close_sockets function
d3e7921 
* Enable checking and closing CLOSE_WAIT sockets in xmlrpcapi
@otamachan
Add --close-sockets option to roscore
00d8f73 
* Enable checking and closing CLOSE_WAIT sockets
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jul 6, 2017

The pull request #834 already tries to address the problem described in the issue. The complexity of changes has so far prevented the previous pull request to be merged.

Maybe @mgrrx @mikepurvis @jspricke @furushchev can comment here how this patch interacts with the existing PR.

@furushchev
Copy link
Copy Markdown
Contributor

furushchev commented Jul 7, 2017 •
edited
Loading

This is more elegant way to close CLOSE_WAIT socket.
I tested this patch on a real robot and checked it works well without any trouble at this time (at least no side effect like the one I encountered in the previous pull request.)
+1 to be merged (also I strongly recommend to set this option True by default since it's now clear that the existing ROS system has the bug in master that loses communication among all the nodes.)

@otamachan
Copy link
Copy Markdown
Contributor Author

otamachan commented Jul 7, 2017

Thanks for your comments and feedback. @furushchev

For anyone who is interested in testing this, I prepared the branch to close CLOSE_WAIT sockets by default. lunar-devel...otamachan:fix-610-by-default

@dirk-thomas dirk-thomas mentioned this pull request Jul 18, 2017
Merged
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jul 18, 2017

For anyone who is interested in testing this, I prepared the branch to close CLOSE_WAIT sockets by default. lunar-devel...otamachan:fix-610-by-default

I created #1104 from that branch. This looks very promising since it seems to resolve the current problem as well as does it in a very "simple" way which should not introduce any side effects.

@otamachan
Copy link
Copy Markdown
Contributor Author

otamachan commented Jul 18, 2017

@dirk-thomas Thank you very much for moving this forward.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jul 18, 2017

Thank you for coming up with this much simpler patch!

@otamachan otamachan closed this Jul 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

3 participants

@otamachan @dirk-thomas @furushchev