{"action":"created","issue":{"url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/10","labels_url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/10/labels{/name}","comments_url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/10/comments","events_url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/10/events","html_url":"https://github.com/PTC-ALM/TortoiseSI/issues/10","id":55192598,"number":10,"title":"Connection Handling","user":{"login":"KristinaTaylor","id":10520385,"avatar_url":"https://avatars.githubusercontent.com/u/10520385?v=3","gravatar_id":"","url":"https://api.github.com/users/KristinaTaylor","html_url":"https://github.com/KristinaTaylor","followers_url":"https://api.github.com/users/KristinaTaylor/followers","following_url":"https://api.github.com/users/KristinaTaylor/following{/other_user}","gists_url":"https://api.github.com/users/KristinaTaylor/gists{/gist_id}","starred_url":"https://api.github.com/users/KristinaTaylor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/KristinaTaylor/subscriptions","organizations_url":"https://api.github.com/users/KristinaTaylor/orgs","repos_url":"https://api.github.com/users/KristinaTaylor/repos","events_url":"https://api.github.com/users/KristinaTaylor/events{/privacy}","received_events_url":"https://api.github.com/users/KristinaTaylor/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/labels/enhancement","name":"enhancement","color":"84b6eb"},{"url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/labels/needs%20docs","name":"needs docs","color":"d4c5f9"}],"state":"open","locked":false,"assignee":{"login":"atusek","id":6186615,"avatar_url":"https://avatars.githubusercontent.com/u/6186615?v=3","gravatar_id":"","url":"https://api.github.com/users/atusek","html_url":"https://github.com/atusek","followers_url":"https://api.github.com/users/atusek/followers","following_url":"https://api.github.com/users/atusek/following{/other_user}","gists_url":"https://api.github.com/users/atusek/gists{/gist_id}","starred_url":"https://api.github.com/users/atusek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/atusek/subscriptions","organizations_url":"https://api.github.com/users/atusek/orgs","repos_url":"https://api.github.com/users/atusek/repos","events_url":"https://api.github.com/users/atusek/events{/privacy}","received_events_url":"https://api.github.com/users/atusek/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/milestones/9","html_url":"https://github.com/PTC-ALM/TortoiseSI/milestones/Sprint8","labels_url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/milestones/9/labels","id":1013080,"number":9,"title":"Sprint8","description":"","creator":{"login":"KristinaTaylor","id":10520385,"avatar_url":"https://avatars.githubusercontent.com/u/10520385?v=3","gravatar_id":"","url":"https://api.github.com/users/KristinaTaylor","html_url":"https://github.com/KristinaTaylor","followers_url":"https://api.github.com/users/KristinaTaylor/followers","following_url":"https://api.github.com/users/KristinaTaylor/following{/other_user}","gists_url":"https://api.github.com/users/KristinaTaylor/gists{/gist_id}","starred_url":"https://api.github.com/users/KristinaTaylor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/KristinaTaylor/subscriptions","organizations_url":"https://api.github.com/users/KristinaTaylor/orgs","repos_url":"https://api.github.com/users/KristinaTaylor/repos","events_url":"https://api.github.com/users/KristinaTaylor/events{/privacy}","received_events_url":"https://api.github.com/users/KristinaTaylor/received_events","type":"User","site_admin":false},"open_issues":4,"closed_issues":0,"state":"open","created_at":"2015-03-09T17:58:03Z","updated_at":"2015-03-13T19:02:56Z","due_on":"2015-03-19T04:00:00Z","closed_at":null},"comments":7,"created_at":"2015-01-22T18:49:15Z","updated_at":"2015-03-19T13:14:41Z","closed_at":null,"body":"When the user first starts Windows, the client is typically not running. \r\n\r\nWhen Windows first asks for status, the Integrity client will be automatically launched.\r\n\r\nWhen Windows asks for status on a file, a connection will be required, and the Integrity client will attempt a connection, using the method defined by the client preferences (prompt or no prompt).\r\n\r\nIf the client fails to connect, the integration will go into a \"silent\" offline mode.  To go online, a user must either explicitly connect via the Integrity Client, or do an explicit gesture.\r\n\r\nPeriodic status (implicit) operations (following the pattern established by wf folders), the integration should check si servers for an active connection.\r\n"},"comment":{"url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/comments/83558248","html_url":"https://github.com/PTC-ALM/TortoiseSI/issues/10#issuecomment-83558248","issue_url":"https://api.github.com/repos/PTC-ALM/TortoiseSI/issues/10","id":83558248,"user":{"login":"atusek","id":6186615,"avatar_url":"https://avatars.githubusercontent.com/u/6186615?v=3","gravatar_id":"","url":"https://api.github.com/users/atusek","html_url":"https://github.com/atusek","followers_url":"https://api.github.com/users/atusek/followers","following_url":"https://api.github.com/users/atusek/following{/other_user}","gists_url":"https://api.github.com/users/atusek/gists{/gist_id}","starred_url":"https://api.github.com/users/atusek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/atusek/subscriptions","organizations_url":"https://api.github.com/users/atusek/orgs","repos_url":"https://api.github.com/users/atusek/repos","events_url":"https://api.github.com/users/atusek/events{/privacy}","received_events_url":"https://api.github.com/users/atusek/received_events","type":"User","site_admin":false},"created_at":"2015-03-19T13:14:41Z","updated_at":"2015-03-19T13:14:41Z","body":"Including email trail discussion around this issue:\r\n\r\n------------------------------------------\r\nFrom: Taylor, Kristina \r\nSent: March-17-15 11:51 AM\r\nTo: Leskowsky, Blaise; Tusek, Andrej; Fleming, Gordon\r\nCc: Reid, Randall\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nI think I’m ok with the fix being to remove the code, based on what I’m seeing below. \r\n\r\nCC’ing Randall for his awareness of the issue/resolution.\r\n\r\nThank you for doing this investigation. If we find evidence of a memory leak in the future, we can try to resolve some other way that doesn’t make it crash. \r\n\r\nKristina\r\n\r\nFrom: Leskowsky, Blaise \r\nSent: March-17-15 11:26 AM\r\nTo: Tusek, Andrej; Fleming, Gordon; Taylor, Kristina\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nNote that this is still not a reliable indicator of how much memory the executable is actually using, but it provides a reasonable approximation of it, that is it cannot prove the existence of a leak or not.\r\n\r\nFrom: Leskowsky, Blaise \r\nSent: Tuesday, March 17, 2015 11:23 AM\r\nTo: Tusek, Andrej; Fleming, Gordon; Taylor, Kristina\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nSome observations using perfmon. Tested using a large data set, 1 member and 1000 adds, which all require calls to wf fileinfo. Blue line represents Private Bytes of explorer process; pinkish line, Virtual Bytes. In terms of memory leak, the private bytes behaviour is more important. Growth in private bytes could mean memory leak, but apparently it could also mean memory fragmentation, so we should take care in how we want to interpret this data.\r\n\r\nRough event time breakdowns:\r\n1.\t10:52:35 – 10:54:55 – Start up explorer but don’t  navigate to sandbox with large data set yet. Private bytes around 48.5 MB. Make sure disconnected (offline), navigate to sandbox directory and keep hitting F5. Event viewer shows some status timeout warnings at this time.\r\n2.\t10:54:55 – 10:56:05 – do nothing in explorer\r\n3.\t10:56:05 – 10:58:25 – keep hitting F5\r\n4.\t10:58:25 – onwards – connect (online) and refresh a few times. Statues now show up. Hitting F5 after this time, see no significant increase in memory usage. \r\n\r\n![memorytest](https://cloud.githubusercontent.com/assets/6186615/6731060/439ee7e6-ce18-11e4-8fbe-8ca1aaac47ef.jpg)\r\n\r\n\r\nFrom: Tusek, Andrej \r\nSent: Tuesday, March 17, 2015 9:31 AM\r\nTo: Fleming, Gordon; Leskowsky, Blaise; Taylor, Kristina\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nAgreed – we didn’t seen any evidence of memory leaks when this code was removed.\r\n\r\nBlaise – you mentioned that you were seeing crashes yesterday. Are you able to do some testing with the code removed?\r\n\r\nFrom: Fleming, Gordon \r\nSent: March-17-15 9:17 AM\r\nTo: Leskowsky, Blaise; Taylor, Kristina\r\nCc: Tusek, Andrej\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nI don’t think it is an issue based on our previous observations.  This method is called many times and when we lower the timeout to 1 second to force timeouts we are not seeing any increased memory usage by Windows explorer or any lingering network connections (which could be an indication that the smart pointer is never being freed).\r\n\r\nAccording to the comment, it looks like Gilad was worried that that unique_ptr (smart pointer) would not be instantiated because it is being passed as a template parameter, i.e.  there is no guarantee that the std::async implementation instantiates this object before the timeout occurs.  From my perspective, the smart pointer is either instantiated or it is not and in either case we are fine.  If it is not instantiated then there is nothing to free and if it is instantiated then the smart pointer will free the IntegrityResponse object when it goes out of scope.\r\n\r\nGordon\r\n\r\nFrom: Leskowsky, Blaise \r\nSent: March-16-15 4:17 PM\r\nTo: Taylor, Kristina\r\nCc: Tusek, Andrej; Fleming, Gordon\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nThat seems to be consistent with what we thought as well.\r\nI’m trying to think how we could prove it formally. It’s tricky to do on a real install since we make the timeout quite large (10 s). We know we can almost force timeouts by bumping that value down to 1 second in our dev environments, although I don’t remember if this works in both online and prompting states. At that point, not sure what we would do. Thoughts, Andrej and Gordon?\r\n\r\nBlaise \r\n\r\nFrom: Taylor, Kristina \r\nSent: Monday, March 16, 2015 4:09 PM\r\nTo: Leskowsky, Blaise\r\nCc: Tusek, Andrej; Fleming, Gordon\r\nSubject: RE: Need to talk to Gilad about this \"workaround\"\r\n\r\nOk, so just based on my quick and dirty interpretation of the question, Gilad’s immediate response was:\r\ni think I was worried about memory leaking. if they can make a dummy program that proves that there is no leak (a little bit tricky to do so, requires the right timing) then it would not be needed. make sure the test forces a time out. and make sure the constructor still gets called in that case… err that should say destructor not constructor.\r\n\r\nI’ll send him the more detailed explanation, just to confirm.\r\n\r\nFrom: Leskowsky, Blaise \r\nSent: March-16-15 3:59 PM\r\nTo: Taylor, Kristina\r\nCc: Tusek, Andrej; Fleming, Gordon\r\nSubject: Need to talk to Gilad about this \"workaround\"\r\n\r\nHello Kristina,\r\n\r\nSo the offending code is in fileInfo() in IntegrityActions.cpp. I have marked where we have seen the Explorer crash occur.\r\nThe problem is with the get() call status timeout handling: this leads to a memory access violation which puts us in the <future> library. We’re not quite sure the reasoning behind why responseFuture.get() specifically needs to be called here since according to the comment, it seems it’s a workaround for some issue we’re not sure of. We think that this call might ensure that the smart pointer will eventually get freed by making sure that references from both threads are decremented in the reference count for responseFuture , but we’re still not sure why we need to call responseFuture.get() when we know the response is not ready and it will lead to a memory access violation.\r\n\r\nSo far we have not observed any memory leaks occurring on fileinfo calls, and any open connections to the integration point eventually expire and close if they do timeout. Could Gilad provide his insight into what the exact purpose of that call is and what scenarios it should avoid?\r\n\r\nIntegrityCommand command(L\"wf\", L\"fileinfo\");\r\n                command.addSelection(file);\r\n\r\n                std::future<std::unique_ptr<IntegrityResponse>> responseFuture =\r\n                                std::async(std::launch::async, [command, &session]() { return session.execute(command); });\r\n\r\n                std::future_status status = responseFuture.wait_for(std::chrono::seconds(10));\r\n                if (status != std::future_status::ready) {\r\n                                EventLog::writeWarning(L\"get status timeout for \" + file);\r\n\r\n                                // need to call get so that the unique_ptr will exisit somewhere and go out of \r\n                                // scope and delete the command runner / response\r\n                                std::async(std::launch::async, [&]{ \r\n                                                                std::unique_ptr<IntegrityResponse> response = responseFuture.get(); \r\n                                                }); // Windows explorer will crash here\r\n\r\n                                return (FileStatusFlags)FileStatus::TimeoutError;\r\n                }\r\n\r\n                // TODO check future's exception... ?\r\n                std::unique_ptr<IntegrityResponse> response = responseFuture.get();\r\n                if (response == NULL) {\r\n                                EventLog::writeWarning(L\"get status failed to return response\");\r\n                                return NO_STATUS;\r\n                }\r\n\r\nGordon and Andrej, feel free to jump in and correct anything incorrect I may have said here regarding the nature of the problem.\r\n\r\nThanks,\r\n\r\nBlaise\r\n"}}