{"action":"created","comment":{"url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/comments/50138258","id":50138258,"diff_hunk":"@@ -77,22 +77,28 @@ void PVRecord::destroy()\n         isDestroyed = true;\n         pvTimeStamp.detach();\n     \n-        std::list<PVRecordClientPtr>::iterator clientIter;\n+        std::list<PVRecordClientWPtr>::iterator clientIter;\n         while(true) {\n             clientIter = pvRecordClientList.begin();\n             if(clientIter==pvRecordClientList.end()) break;\n+            PVRecordClientPtr client = clientIter->lock();\n+            if(client.get()) {\n+                unlock();\n+                client->detach(getPtrSelf());\n+                lock();\n+            }\n             pvRecordClientList.erase(clientIter);","path":"src/database/pvRecord.cpp","position":15,"original_position":15,"commit_id":"4ac867ec0f2e0d96c57f10e25ecbcccc2f2f62cd","original_commit_id":"4ac867ec0f2e0d96c57f10e25ecbcccc2f2f62cd","user":{"login":"mdavidsaver","id":4429215,"avatar_url":"https://avatars.githubusercontent.com/u/4429215?v=3","gravatar_id":"","url":"https://api.github.com/users/mdavidsaver","html_url":"https://github.com/mdavidsaver","followers_url":"https://api.github.com/users/mdavidsaver/followers","following_url":"https://api.github.com/users/mdavidsaver/following{/other_user}","gists_url":"https://api.github.com/users/mdavidsaver/gists{/gist_id}","starred_url":"https://api.github.com/users/mdavidsaver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mdavidsaver/subscriptions","organizations_url":"https://api.github.com/users/mdavidsaver/orgs","repos_url":"https://api.github.com/users/mdavidsaver/repos","events_url":"https://api.github.com/users/mdavidsaver/events{/privacy}","received_events_url":"https://api.github.com/users/mdavidsaver/received_events","type":"User","site_admin":false},"body":"The most obvious issue here is that explicit unlock()/lock() leaves open the possibility that an exception thrown be 'client->detach()' would no re-lock.  There are several ways to correct this.  One is explicit try/catch/rethrow.\r\n\r\n```c++\r\nif(client) {\r\n  unlock();\r\n  try {\r\n    client->detach(getPtrSelf());\r\n    lock();\r\n  } catch(...) {\r\n    lock();\r\n    throw;\r\n  }\r\n}\r\n```\r\nI personally find this verbose and potentially error prone as the re-lock() call should be duplicated.  My preference is to add a \"guard\" class to handle unlock() and re-lock().  So a definition is added to some header.\r\n\r\n```c++\r\ntemplate<typename T>\r\nstruct XUnLocker {\r\n  T* ref;\r\n  XUnLocker(T& r) :ref(&r) { r.unlock(); }\r\n ~XUnLocker() { r.lock(); }\r\n};\r\n```\r\nThen the code in question becomes:\r\n\r\n```c++\r\nif(client) {\r\n  XUnLocker<PVRecord> L(*this); // calls unlock()\r\n  client->detach(getPtrSelf());\r\n  // lock() when 'L' goes out of scope due to normal flow or exception unwind\r\n}\r\n```","created_at":"2016-01-19T16:47:30Z","updated_at":"2016-01-19T16:47:30Z","html_url":"https://github.com/epics-base/pvDatabaseCPP/pull/4#discussion_r50138258","pull_request_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4","_links":{"self":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/comments/50138258"},"html":{"href":"https://github.com/epics-base/pvDatabaseCPP/pull/4#discussion_r50138258"},"pull_request":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4"}}},"pull_request":{"url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4","id":55784845,"html_url":"https://github.com/epics-base/pvDatabaseCPP/pull/4","diff_url":"https://github.com/epics-base/pvDatabaseCPP/pull/4.diff","patch_url":"https://github.com/epics-base/pvDatabaseCPP/pull/4.patch","issue_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/4","number":4,"state":"open","locked":false,"title":"use weak pointer for callbacks","user":{"login":"mrkraimer","id":6115542,"avatar_url":"https://avatars.githubusercontent.com/u/6115542?v=3","gravatar_id":"","url":"https://api.github.com/users/mrkraimer","html_url":"https://github.com/mrkraimer","followers_url":"https://api.github.com/users/mrkraimer/followers","following_url":"https://api.github.com/users/mrkraimer/following{/other_user}","gists_url":"https://api.github.com/users/mrkraimer/gists{/gist_id}","starred_url":"https://api.github.com/users/mrkraimer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mrkraimer/subscriptions","organizations_url":"https://api.github.com/users/mrkraimer/orgs","repos_url":"https://api.github.com/users/mrkraimer/repos","events_url":"https://api.github.com/users/mrkraimer/events{/privacy}","received_events_url":"https://api.github.com/users/mrkraimer/received_events","type":"User","site_admin":false},"body":"Use weak pointers for callbacks.","created_at":"2016-01-12T16:51:23Z","updated_at":"2016-01-19T16:47:30Z","closed_at":null,"merged_at":null,"merge_commit_sha":"e1e5456ec87058663c04442ff95faca608fd8186","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4/commits","review_comments_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4/comments","review_comment_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/comments{/number}","comments_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/4/comments","statuses_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/statuses/4ac867ec0f2e0d96c57f10e25ecbcccc2f2f62cd","head":{"label":"mrkraimer:master","ref":"master","sha":"4ac867ec0f2e0d96c57f10e25ecbcccc2f2f62cd","user":{"login":"mrkraimer","id":6115542,"avatar_url":"https://avatars.githubusercontent.com/u/6115542?v=3","gravatar_id":"","url":"https://api.github.com/users/mrkraimer","html_url":"https://github.com/mrkraimer","followers_url":"https://api.github.com/users/mrkraimer/followers","following_url":"https://api.github.com/users/mrkraimer/following{/other_user}","gists_url":"https://api.github.com/users/mrkraimer/gists{/gist_id}","starred_url":"https://api.github.com/users/mrkraimer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mrkraimer/subscriptions","organizations_url":"https://api.github.com/users/mrkraimer/orgs","repos_url":"https://api.github.com/users/mrkraimer/repos","events_url":"https://api.github.com/users/mrkraimer/events{/privacy}","received_events_url":"https://api.github.com/users/mrkraimer/received_events","type":"User","site_admin":false},"repo":{"id":39834781,"name":"pvDatabaseCPP","full_name":"mrkraimer/pvDatabaseCPP","owner":{"login":"mrkraimer","id":6115542,"avatar_url":"https://avatars.githubusercontent.com/u/6115542?v=3","gravatar_id":"","url":"https://api.github.com/users/mrkraimer","html_url":"https://github.com/mrkraimer","followers_url":"https://api.github.com/users/mrkraimer/followers","following_url":"https://api.github.com/users/mrkraimer/following{/other_user}","gists_url":"https://api.github.com/users/mrkraimer/gists{/gist_id}","starred_url":"https://api.github.com/users/mrkraimer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mrkraimer/subscriptions","organizations_url":"https://api.github.com/users/mrkraimer/orgs","repos_url":"https://api.github.com/users/mrkraimer/repos","events_url":"https://api.github.com/users/mrkraimer/events{/privacy}","received_events_url":"https://api.github.com/users/mrkraimer/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/mrkraimer/pvDatabaseCPP","description":"","fork":true,"url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP","forks_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/forks","keys_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/keys{/key_id}","collaborators_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/teams","hooks_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/hooks","issue_events_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/issues/events{/number}","events_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/events","assignees_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/assignees{/user}","branches_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/branches{/branch}","tags_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/tags","blobs_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/git/refs{/sha}","trees_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/git/trees{/sha}","statuses_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/statuses/{sha}","languages_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/languages","stargazers_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/stargazers","contributors_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/contributors","subscribers_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/subscribers","subscription_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/subscription","commits_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/commits{/sha}","git_commits_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/git/commits{/sha}","comments_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/comments{/number}","issue_comment_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/issues/comments{/number}","contents_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/contents/{+path}","compare_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/compare/{base}...{head}","merges_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/merges","archive_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/downloads","issues_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/issues{/number}","pulls_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/pulls{/number}","milestones_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/milestones{/number}","notifications_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/labels{/name}","releases_url":"https://api.github.com/repos/mrkraimer/pvDatabaseCPP/releases{/id}","created_at":"2015-07-28T13:15:15Z","updated_at":"2015-07-28T13:15:15Z","pushed_at":"2016-01-07T12:44:16Z","git_url":"git://github.com/mrkraimer/pvDatabaseCPP.git","ssh_url":"git@github.com:mrkraimer/pvDatabaseCPP.git","clone_url":"https://github.com/mrkraimer/pvDatabaseCPP.git","svn_url":"https://github.com/mrkraimer/pvDatabaseCPP","homepage":null,"size":1566,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"epics-base:master","ref":"master","sha":"ca3573291ab525dcf96865e98ffa2cf535995c65","user":{"login":"epics-base","id":10711129,"avatar_url":"https://avatars.githubusercontent.com/u/10711129?v=3","gravatar_id":"","url":"https://api.github.com/users/epics-base","html_url":"https://github.com/epics-base","followers_url":"https://api.github.com/users/epics-base/followers","following_url":"https://api.github.com/users/epics-base/following{/other_user}","gists_url":"https://api.github.com/users/epics-base/gists{/gist_id}","starred_url":"https://api.github.com/users/epics-base/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/epics-base/subscriptions","organizations_url":"https://api.github.com/users/epics-base/orgs","repos_url":"https://api.github.com/users/epics-base/repos","events_url":"https://api.github.com/users/epics-base/events{/privacy}","received_events_url":"https://api.github.com/users/epics-base/received_events","type":"Organization","site_admin":false},"repo":{"id":36086827,"name":"pvDatabaseCPP","full_name":"epics-base/pvDatabaseCPP","owner":{"login":"epics-base","id":10711129,"avatar_url":"https://avatars.githubusercontent.com/u/10711129?v=3","gravatar_id":"","url":"https://api.github.com/users/epics-base","html_url":"https://github.com/epics-base","followers_url":"https://api.github.com/users/epics-base/followers","following_url":"https://api.github.com/users/epics-base/following{/other_user}","gists_url":"https://api.github.com/users/epics-base/gists{/gist_id}","starred_url":"https://api.github.com/users/epics-base/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/epics-base/subscriptions","organizations_url":"https://api.github.com/users/epics-base/orgs","repos_url":"https://api.github.com/users/epics-base/repos","events_url":"https://api.github.com/users/epics-base/events{/privacy}","received_events_url":"https://api.github.com/users/epics-base/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/epics-base/pvDatabaseCPP","description":"","fork":false,"url":"https://api.github.com/repos/epics-base/pvDatabaseCPP","forks_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/forks","keys_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/keys{/key_id}","collaborators_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/teams","hooks_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/hooks","issue_events_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/events{/number}","events_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/events","assignees_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/assignees{/user}","branches_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/branches{/branch}","tags_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/tags","blobs_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/git/refs{/sha}","trees_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/git/trees{/sha}","statuses_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/statuses/{sha}","languages_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/languages","stargazers_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/stargazers","contributors_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/contributors","subscribers_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/subscribers","subscription_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/subscription","commits_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/commits{/sha}","git_commits_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/git/commits{/sha}","comments_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/comments{/number}","issue_comment_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/comments{/number}","contents_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/contents/{+path}","compare_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/compare/{base}...{head}","merges_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/merges","archive_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/downloads","issues_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues{/number}","pulls_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls{/number}","milestones_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/milestones{/number}","notifications_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/labels{/name}","releases_url":"https://api.github.com/repos/epics-base/pvDatabaseCPP/releases{/id}","created_at":"2015-05-22T17:48:05Z","updated_at":"2015-05-22T17:48:41Z","pushed_at":"2016-01-12T16:51:23Z","git_url":"git://github.com/epics-base/pvDatabaseCPP.git","ssh_url":"git@github.com:epics-base/pvDatabaseCPP.git","clone_url":"https://github.com/epics-base/pvDatabaseCPP.git","svn_url":"https://github.com/epics-base/pvDatabaseCPP","homepage":null,"size":1568,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":2,"mirror_url":null,"open_issues_count":1,"forks":2,"open_issues":1,"watchers":0,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4"},"html":{"href":"https://github.com/epics-base/pvDatabaseCPP/pull/4"},"issue":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/4"},"comments":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/issues/4/comments"},"review_comments":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4/comments"},"review_comment":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/pulls/4/commits"},"statuses":{"href":"https://api.github.com/repos/epics-base/pvDatabaseCPP/statuses/4ac867ec0f2e0d96c57f10e25ecbcccc2f2f62cd"}}}}