{"action":"created","issue":{"url":"https://api.github.com/repos/robotology/yarp/issues/828","repository_url":"https://api.github.com/repos/robotology/yarp","labels_url":"https://api.github.com/repos/robotology/yarp/issues/828/labels{/name}","comments_url":"https://api.github.com/repos/robotology/yarp/issues/828/comments","events_url":"https://api.github.com/repos/robotology/yarp/issues/828/events","html_url":"https://github.com/robotology/yarp/pull/828","id":165847554,"number":828,"title":"Fix #826","user":{"login":"Tobias-Fischer","id":5497832,"avatar_url":"https://avatars.githubusercontent.com/u/5497832?v=3","gravatar_id":"","url":"https://api.github.com/users/Tobias-Fischer","html_url":"https://github.com/Tobias-Fischer","followers_url":"https://api.github.com/users/Tobias-Fischer/followers","following_url":"https://api.github.com/users/Tobias-Fischer/following{/other_user}","gists_url":"https://api.github.com/users/Tobias-Fischer/gists{/gist_id}","starred_url":"https://api.github.com/users/Tobias-Fischer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Tobias-Fischer/subscriptions","organizations_url":"https://api.github.com/users/Tobias-Fischer/orgs","repos_url":"https://api.github.com/users/Tobias-Fischer/repos","events_url":"https://api.github.com/users/Tobias-Fischer/events{/privacy}","received_events_url":"https://api.github.com/users/Tobias-Fischer/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/robotology/yarp/labels/Component:%20Library%20-%20YARP_OS","name":"Component: Library - YARP_OS","color":"207de5"},{"url":"https://api.github.com/repos/robotology/yarp/labels/Component:%20ROS%20Integration","name":"Component: ROS Integration","color":"bfe5bf"},{"url":"https://api.github.com/repos/robotology/yarp/labels/Status:%20In%20Progress","name":"Status: In Progress","color":"ededed"}],"state":"open","locked":false,"assignee":{"login":"drdanz","id":1100056,"avatar_url":"https://avatars.githubusercontent.com/u/1100056?v=3","gravatar_id":"","url":"https://api.github.com/users/drdanz","html_url":"https://github.com/drdanz","followers_url":"https://api.github.com/users/drdanz/followers","following_url":"https://api.github.com/users/drdanz/following{/other_user}","gists_url":"https://api.github.com/users/drdanz/gists{/gist_id}","starred_url":"https://api.github.com/users/drdanz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drdanz/subscriptions","organizations_url":"https://api.github.com/users/drdanz/orgs","repos_url":"https://api.github.com/users/drdanz/repos","events_url":"https://api.github.com/users/drdanz/events{/privacy}","received_events_url":"https://api.github.com/users/drdanz/received_events","type":"User","site_admin":false},"assignees":[{"login":"drdanz","id":1100056,"avatar_url":"https://avatars.githubusercontent.com/u/1100056?v=3","gravatar_id":"","url":"https://api.github.com/users/drdanz","html_url":"https://github.com/drdanz","followers_url":"https://api.github.com/users/drdanz/followers","following_url":"https://api.github.com/users/drdanz/following{/other_user}","gists_url":"https://api.github.com/users/drdanz/gists{/gist_id}","starred_url":"https://api.github.com/users/drdanz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drdanz/subscriptions","organizations_url":"https://api.github.com/users/drdanz/orgs","repos_url":"https://api.github.com/users/drdanz/repos","events_url":"https://api.github.com/users/drdanz/events{/privacy}","received_events_url":"https://api.github.com/users/drdanz/received_events","type":"User","site_admin":false},{"login":"Tobias-Fischer","id":5497832,"avatar_url":"https://avatars.githubusercontent.com/u/5497832?v=3","gravatar_id":"","url":"https://api.github.com/users/Tobias-Fischer","html_url":"https://github.com/Tobias-Fischer","followers_url":"https://api.github.com/users/Tobias-Fischer/followers","following_url":"https://api.github.com/users/Tobias-Fischer/following{/other_user}","gists_url":"https://api.github.com/users/Tobias-Fischer/gists{/gist_id}","starred_url":"https://api.github.com/users/Tobias-Fischer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Tobias-Fischer/subscriptions","organizations_url":"https://api.github.com/users/Tobias-Fischer/orgs","repos_url":"https://api.github.com/users/Tobias-Fischer/repos","events_url":"https://api.github.com/users/Tobias-Fischer/events{/privacy}","received_events_url":"https://api.github.com/users/Tobias-Fischer/received_events","type":"User","site_admin":false}],"milestone":null,"comments":6,"created_at":"2016-07-15T18:17:26Z","updated_at":"2016-07-18T11:45:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/robotology/yarp/pulls/828","html_url":"https://github.com/robotology/yarp/pull/828","diff_url":"https://github.com/robotology/yarp/pull/828.diff","patch_url":"https://github.com/robotology/yarp/pull/828.patch"},"body":"See #826\r\nMain contribution is the proper implementation of getBusInfo, which provides a connection list to ROS. It's not yet implemented for services.\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/robotology/yarp/pull/828%23discussion_r71039573%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23discussion_r71040350%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23discussion_r71071743%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233291252%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233294365%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233294536%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233296950%22%2C%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233302452%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/robotology/yarp/pull/828%23issuecomment-233291252%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Rebased%20over%20latest%20%60devel%60%20since%20the%20builds%20were%20failing%20due%20to%20the%20issue%20in%20%60Contact%60%22%2C%20%22created_at%22%3A%20%222016-07-18T10%3A09%3A12Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1100056%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drdanz%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40Tobias-Fischer%20The%20patch%20looks%20good%2C%20thanks%21%5Cr%5Cn%5Cr%5CnI%27m%20just%20not%20sure%20about%20the%20reason%20why%20the%20%5C%22Use%20full%20name%20when%20%28un-%29registering%20publisher%20with%20ROS%5C%22%20commit%20is%20required%2C%20can%20you%20explain%20it%20to%20me%3F%5Cr%5Cn%5Cr%5CnAlso%20I%20think%20you%20are%20leaking%20the%20%60ROSReport%2A%60%2C%20I%20don%27t%20see%20the%20%60delete%60%20and%20%60Contactable%60%20destructor%20does%20not%20delete%20it.%22%2C%20%22created_at%22%3A%20%222016-07-18T10%3A26%3A41Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1100056%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drdanz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Thanks%20%40Tobias-Fischer%21%22%2C%20%22created_at%22%3A%20%222016-07-18T10%3A27%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1403333%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lornat75%22%7D%7D%2C%20%7B%22body%22%3A%20%22Hi%20%40drdanz%2C%5Cr%5Cnregarding%20the%20%5C%22full%20name%5C%22%20commit.%20I%20have%20no%20idea%20about%20the%20internals%2C%20but%20this%20is%20what%20happens%20if%20the%20commit%20is%20not%20applied%3A%5Cr%5Cn%60%60%60%5Cr%5CnWARNING%20The%20following%20nodes%20are%20unexpectedly%20connected%3A%5Cr%5Cn%20%2A%20/rostopic_23303_1468838229656-%3E/chatter-%40/yarp/listener%20%28/chatter%29%5Cr%5Cn%20%2A%20unknown%20%28http%3A//icubunicorn%3A43247/%29-%3E/rosout%20%28/rosout%29%5Cr%5Cn%60%60%60%5Cr%5CnAfter%20applying%20the%20commit%2C%20%60roswtf%60%20does%20not%20complain%20anymore.%20I%20guess%20that%20there%20is%20a%20non-consistency%20with%20some%20of%20the%20values%20yarp%20sends%20to%20ROS%20via%20the%20Slave%20API%20if%20the%20commit%20is%20not%20applied.%20However%20I%20did%20not%20investigate%20further%20as%20the%20commit%20fixes%20it%20nicely.%22%2C%20%22created_at%22%3A%20%222016-07-18T10%3A42%3A26Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5497832%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/Tobias-Fischer%22%7D%7D%2C%20%7B%22body%22%3A%20%22Regarding%20the%20memory%20leak%3A%20I%20now%20delete%20the%20objects%20which%20are%20being%20created%20in%20the%20destructor.%20However%2C%20I%20am%20afraid%20there%20is%20still%20a%20memory%20leak%20elsewhere.%20The%20object%20should%20also%20be%20deleted%20within%20%60void%20yarp%3A%3Aos%3A%3ANode%3A%3AHelper%3A%3Aremove%60%2C%20however%20if%20I%20do%20this%2C%20we%20encounter%20a%20segfault%20later%20on%2C%20as%20the%20%60report%60%20method%20is%20still%20being%20called%20on%20an%20object%20which%20cannot%20be%20dereferenced.%5Cr%5CnI%20think%20what%20we%20need%20is%20a%20%60resetReporter%60%20method%2C%20which%20resets%20the%20%60eventReporter%60%20to%20%60NULL%60.%20What%20do%20you%20think%20%40drdanz%3F%22%2C%20%22created_at%22%3A%20%222016-07-18T11%3A16%3A19Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5497832%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/Tobias-Fischer%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%207f31fdc72148142e20514ce5b2622534a064cb8e%20src/libYARP_OS/src/Node.cpp%2028%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/robotology/yarp/pull/828%23discussion_r71071743%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20am%20not%20sure%20whether%20the%20%60toURI%60%20method%20should%20return%20a%20%5C%22/%5C%22%20at%20the%20end%20by%20default.%22%2C%20%22created_at%22%3A%20%222016-07-16T20%3A56%3A20Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5497832%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/Tobias-Fischer%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/libYARP_OS/src/Node.cpp%3AL25-63%22%7D%2C%20%22Pull%20325dcf4e5f6237bfc2f073a9b4f25e5d9dca7a2a%20src/libYARP_OS/src/Node.cpp%204%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/robotology/yarp/pull/828%23discussion_r71039573%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%20do%20not%20use%20LogStream%20in%20YARP_OS%20port%20related%20classes.%20Since%20log%20generated%20by%20yDebug%20etc.%20can%20be%20forwarded%20to%20the%20logger%20using%20a%20port%2C%20it%20is%20better%20for%20now%20to%20avoid%20using%20them%20around%20these%20classes%2C%20as%20this%20might%20cause%20recursive%20loops...%5Cr%5CnUnfortunately%20we%20have%20a%20lots%20of%20logging%20methods%20around%20the%20YARP%20code...%20One%20day%20we%20will%20make%20disappear%20them%20all%20and%20leave%20only%20one%2C%20but%20for%20now%20have%20a%20look%20at%20what%20is%20already%20used%20in%20the%20class%20and%20use%20the%20same...%22%2C%20%22created_at%22%3A%20%222016-07-15T20%3A55%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1100056%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drdanz%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20rest%20of%20the%20review%20on%20Monday%20%3Awink%3A%22%2C%20%22created_at%22%3A%20%222016-07-15T21%3A01%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1100056%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drdanz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/libYARP_OS/src/Node.cpp%3AL9-21%22%7D%7D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n- [x] <a href='#crh-comment-Pull 325dcf4e5f6237bfc2f073a9b4f25e5d9dca7a2a src/libYARP_OS/src/Node.cpp 4'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/robotology/yarp/pull/828#discussion_r71039573'>File: src/libYARP_OS/src/Node.cpp:L9-21</a></b>\n- <a href='https://github.com/drdanz'><img border=0 src='https://avatars.githubusercontent.com/u/1100056?v=3' height=16 width=16'></a> Please do not use LogStream in YARP_OS port related classes. Since log generated by yDebug etc. can be forwarded to the logger using a port, it is better for now to avoid using them around these classes, as this might cause recursive loops...\nUnfortunately we have a lots of logging methods around the YARP code... One day we will make disappear them all and leave only one, but for now have a look at what is already used in the class and use the same...\n- <a href='https://github.com/drdanz'><img border=0 src='https://avatars.githubusercontent.com/u/1100056?v=3' height=16 width=16'></a> The rest of the review on Monday :wink:\n- [ ] <a href='#crh-comment-Pull 7f31fdc72148142e20514ce5b2622534a064cb8e src/libYARP_OS/src/Node.cpp 28'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/robotology/yarp/pull/828#discussion_r71071743'>File: src/libYARP_OS/src/Node.cpp:L25-63</a></b>\n- <a href='https://github.com/Tobias-Fischer'><img border=0 src='https://avatars.githubusercontent.com/u/5497832?v=3' height=16 width=16'></a> I am not sure whether the `toURI` method should return a \"/\" at the end by default.\n- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/robotology/yarp/pull/828#issuecomment-233291252'>General Comment</a></b>\n- <a href='https://github.com/drdanz'><img border=0 src='https://avatars.githubusercontent.com/u/1100056?v=3' height=16 width=16'></a> Rebased over latest `devel` since the builds were failing due to the issue in `Contact`\n- <a href='https://github.com/drdanz'><img border=0 src='https://avatars.githubusercontent.com/u/1100056?v=3' height=16 width=16'></a> @Tobias-Fischer The patch looks good, thanks!\nI'm just not sure about the reason why the \"Use full name when (un-)registering publisher with ROS\" commit is required, can you explain it to me?\nAlso I think you are leaking the `ROSReport*`, I don't see the `delete` and `Contactable` destructor does not delete it.\n- <a href='https://github.com/lornat75'><img border=0 src='https://avatars.githubusercontent.com/u/1403333?v=3' height=16 width=16'></a> Thanks @Tobias-Fischer!\n- <a href='https://github.com/Tobias-Fischer'><img border=0 src='https://avatars.githubusercontent.com/u/5497832?v=3' height=16 width=16'></a> Hi @drdanz,\nregarding the \"full name\" commit. I have no idea about the internals, but this is what happens if the commit is not applied:\n```\nWARNING The following nodes are unexpectedly connected:\n* /rostopic_23303_1468838229656->/chatter-@/yarp/listener (/chatter)\n* unknown (http://icubunicorn:43247/)->/rosout (/rosout)\n```\nAfter applying the commit, `roswtf` does not complain anymore. I guess that there is a non-consistency with some of the values yarp sends to ROS via the Slave API if the commit is not applied. However I did not investigate further as the commit fixes it nicely.\n- <a href='https://github.com/Tobias-Fischer'><img border=0 src='https://avatars.githubusercontent.com/u/5497832?v=3' height=16 width=16'></a> Regarding the memory leak: I now delete the objects which are being created in the destructor. However, I am afraid there is still a memory leak elsewhere. The object should also be deleted within `void yarp::os::Node::Helper::remove`, however if I do this, we encounter a segfault later on, as the `report` method is still being called on an object which cannot be dereferenced.\nI think what we need is a `resetReporter` method, which resets the `eventReporter` to `NULL`. What do you think @drdanz?\n\n\n<a href='https://www.codereviewhub.com/robotology/yarp/pull/828?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/robotology/yarp/pull/828?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/robotology/yarp/pull/828'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/robotology/yarp/issues/comments/233307189","html_url":"https://github.com/robotology/yarp/pull/828#issuecomment-233307189","issue_url":"https://api.github.com/repos/robotology/yarp/issues/828","id":233307189,"user":{"login":"drdanz","id":1100056,"avatar_url":"https://avatars.githubusercontent.com/u/1100056?v=3","gravatar_id":"","url":"https://api.github.com/users/drdanz","html_url":"https://github.com/drdanz","followers_url":"https://api.github.com/users/drdanz/followers","following_url":"https://api.github.com/users/drdanz/following{/other_user}","gists_url":"https://api.github.com/users/drdanz/gists{/gist_id}","starred_url":"https://api.github.com/users/drdanz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drdanz/subscriptions","organizations_url":"https://api.github.com/users/drdanz/orgs","repos_url":"https://api.github.com/users/drdanz/repos","events_url":"https://api.github.com/users/drdanz/events{/privacy}","received_events_url":"https://api.github.com/users/drdanz/received_events","type":"User","site_admin":false},"created_at":"2016-07-18T11:45:29Z","updated_at":"2016-07-18T11:45:29Z","body":"@Tobias-Fischer Have you tried if `item.contactable->setReporter(NULL);` works?"}}
{"actor":{"display_login":"drdanz"}}