{"action":"created","comment":{"url":"https://api.github.com/repos/highfidelity/hifi/pulls/comments/219926381","pull_request_review_id":158217538,"id":219926381,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDIxOTkyNjM4MQ==","diff_hunk":"@@ -92,32 +92,115 @@ namespace Setting {\n     }\n \n     void Manager::saveAll() {\n-        bool forceSync = false;\n         withWriteLock([&] {\n+            bool forceSync = false;\n             for (auto key : _pendingChanges.keys()) {\n                 auto newValue = _pendingChanges[key];\n-                auto savedValue = value(key, UNSET_VALUE);\n+                auto savedValue = _qSettings.value(key, UNSET_VALUE);\n                 if (newValue == savedValue) {\n                     continue;\n                 }\n+                forceSync = true;\n                 if (newValue == UNSET_VALUE || !newValue.isValid()) {\n-                    forceSync = true;\n-                    remove(key);\n+                    _qSettings.remove(key);\n                 } else {\n-                    forceSync = true;\n-                    setValue(key, newValue);\n+                    _qSettings.setValue(key, newValue);\n                 }\n             }\n             _pendingChanges.clear();\n-        });\n \n-        if (forceSync) {\n-            sync();\n-        }\n+            if (forceSync) {\n+                _qSettings.sync();\n+            }\n+        });\n \n         // Restart timer\n         if (_saveTimer) {\n             _saveTimer->start();\n         }\n     }\n+\n+    QString Manager::fileName() const {\n+        return resultWithReadLock<QString>([&] {\n+            return _qSettings.fileName();\n+        });\n+    }\n+\n+    void Manager::remove(const QString &key) {\n+        withWriteLock([&] {\n+            _qSettings.remove(key);\n+        });\n+    }\n+\n+    QStringList Manager::childGroups() const {\n+        return resultWithReadLock<QStringList>([&] {\n+            return _qSettings.childGroups();\n+        });\n+    }\n+\n+    QStringList Manager::childKeys() const {\n+        return resultWithReadLock<QStringList>([&] {\n+            return _qSettings.childKeys();\n+        });\n+    }\n+\n+    QStringList Manager::allKeys() const {\n+        return resultWithReadLock<QStringList>([&] {\n+            return _qSettings.allKeys();\n+        });\n+    }\n+\n+    bool Manager::contains(const QString &key) const {\n+        return resultWithReadLock<bool>([&] {\n+            return _qSettings.contains(key);\n+        });\n+    }\n+\n+    int Manager::beginReadArray(const QString &prefix) {\n+        return resultWithReadLock<int>([&] {\n+            return _qSettings.beginReadArray(prefix);\n+        });\n+    }","path":"libraries/shared/src/SettingManager.cpp","position":91,"original_position":91,"commit_id":"86e502637e581d6cdd9e5e6cc48cb37215891dd7","original_commit_id":"86e502637e581d6cdd9e5e6cc48cb37215891dd7","user":{"login":"Atlante45","id":4036492,"node_id":"MDQ6VXNlcjQwMzY0OTI=","avatar_url":"https://avatars0.githubusercontent.com/u/4036492?v=4","gravatar_id":"","url":"https://api.github.com/users/Atlante45","html_url":"https://github.com/Atlante45","followers_url":"https://api.github.com/users/Atlante45/followers","following_url":"https://api.github.com/users/Atlante45/following{/other_user}","gists_url":"https://api.github.com/users/Atlante45/gists{/gist_id}","starred_url":"https://api.github.com/users/Atlante45/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Atlante45/subscriptions","organizations_url":"https://api.github.com/users/Atlante45/orgs","repos_url":"https://api.github.com/users/Atlante45/repos","events_url":"https://api.github.com/users/Atlante45/events{/privacy}","received_events_url":"https://api.github.com/users/Atlante45/received_events","type":"User","site_admin":false},"body":"What makes it worse? It's the same behavior it had before, except it won't crash.\r\n\r\nThe calls to `Settings` from threads other than the main thread are really rare, so this makes this issue at least really rare. And the only thing it does is that you lose 1 save of that value from thread 2. Which means that if you crash, that value might be 20sec stale instead of 10sec. The likeliness that that issue happening every time you try to save the value is extremely low.\r\n\r\nMy point is that any effort that goes toward fixing Settings is effort that doesn't go toward removing it. This is good enough given the unlikeliness of those events.\r\n\r\n\r\n**EDIT:** With that said, I took a look at `Settings` and I think there's an easy enough fix we can do right now that will satisfy everyone.\r\nI suggest we revert all the changes made to the `SettingManager` and just leave it as it is.\r\nHowever we can add this the the `Settings` ctor and dtor:\r\n```cpp\r\nSettings::Settings() : _manager(DependencyManager::get<Setting::Manager>()) {\r\n    _manager.getLock().lockForWrite();\r\n}\r\nSettings::~Settings() {\r\n    _manager.getLock().unlock();\r\n}\r\n```\r\nThis will ensure the Manager is locked for the whole duration of whatever operation is happening and since `Settings` calls from different threads are rare, we shouldn't get a lot of lock contention.","created_at":"2018-09-24T17:42:53Z","updated_at":"2018-09-24T17:42:53Z","html_url":"https://github.com/highfidelity/hifi/pull/14010#discussion_r219926381","pull_request_url":"https://api.github.com/repos/highfidelity/hifi/pulls/14010","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/comments/219926381"},"html":{"href":"https://github.com/highfidelity/hifi/pull/14010#discussion_r219926381"},"pull_request":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/14010"}},"in_reply_to_id":219651552},"pull_request":{"url":"https://api.github.com/repos/highfidelity/hifi/pulls/14010","id":215841358,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE1ODQxMzU4","html_url":"https://github.com/highfidelity/hifi/pull/14010","diff_url":"https://github.com/highfidelity/hifi/pull/14010.diff","patch_url":"https://github.com/highfidelity/hifi/pull/14010.patch","issue_url":"https://api.github.com/repos/highfidelity/hifi/issues/14010","number":14010,"state":"open","locked":false,"title":"make settings thread safe","user":{"login":"sethalves","id":1137072,"node_id":"MDQ6VXNlcjExMzcwNzI=","avatar_url":"https://avatars1.githubusercontent.com/u/1137072?v=4","gravatar_id":"","url":"https://api.github.com/users/sethalves","html_url":"https://github.com/sethalves","followers_url":"https://api.github.com/users/sethalves/followers","following_url":"https://api.github.com/users/sethalves/following{/other_user}","gists_url":"https://api.github.com/users/sethalves/gists{/gist_id}","starred_url":"https://api.github.com/users/sethalves/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sethalves/subscriptions","organizations_url":"https://api.github.com/users/sethalves/orgs","repos_url":"https://api.github.com/users/sethalves/repos","events_url":"https://api.github.com/users/sethalves/events{/privacy}","received_events_url":"https://api.github.com/users/sethalves/received_events","type":"User","site_admin":false},"body":"- attempt to fix memory corruption associated with Settings\r\n\r\nhttps://highfidelity.fogbugz.com/f/cases/17459/Interface-json-contains-corrupted-entries\r\nhttps://highfidelity.fogbugz.com/f/cases/17458/Duplicate-fields-in-Interface-json-Settings\r\nhttps://highfidelity.fogbugz.com/f/cases/18463/crash-in-Setting-Manager-saveAll\r\n\r\nhttps://github.com/highfidelity/hifi/pull/14056\r\nhttps://github.com/highfidelity/hifi/pull/14057\r\n","created_at":"2018-09-16T19:42:08Z","updated_at":"2018-09-24T17:42:53Z","closed_at":null,"merged_at":null,"merge_commit_sha":"52ab16468c9673138528b7006fb2840d7a82c9c8","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[{"id":743411186,"node_id":"MDU6TGFiZWw3NDM0MTExODY=","url":"https://api.github.com/repos/highfidelity/hifi/labels/Client","name":"Client","color":"2872fc","default":false},{"id":313595519,"node_id":"MDU6TGFiZWwzMTM1OTU1MTk=","url":"https://api.github.com/repos/highfidelity/hifi/labels/QA%20Ready","name":"QA Ready","color":"bfd4f2","default":false}],"milestone":{"url":"https://api.github.com/repos/highfidelity/hifi/milestones/34","html_url":"https://github.com/highfidelity/hifi/milestone/34","labels_url":"https://api.github.com/repos/highfidelity/hifi/milestones/34/labels","id":3290833,"node_id":"MDk6TWlsZXN0b25lMzI5MDgzMw==","number":34,"title":"v0.74.0","description":"","creator":{"login":"conklin94122","id":12115653,"node_id":"MDQ6VXNlcjEyMTE1NjUz","avatar_url":"https://avatars2.githubusercontent.com/u/12115653?v=4","gravatar_id":"","url":"https://api.github.com/users/conklin94122","html_url":"https://github.com/conklin94122","followers_url":"https://api.github.com/users/conklin94122/followers","following_url":"https://api.github.com/users/conklin94122/following{/other_user}","gists_url":"https://api.github.com/users/conklin94122/gists{/gist_id}","starred_url":"https://api.github.com/users/conklin94122/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/conklin94122/subscriptions","organizations_url":"https://api.github.com/users/conklin94122/orgs","repos_url":"https://api.github.com/users/conklin94122/repos","events_url":"https://api.github.com/users/conklin94122/events{/privacy}","received_events_url":"https://api.github.com/users/conklin94122/received_events","type":"User","site_admin":false},"open_issues":31,"closed_issues":9,"state":"open","created_at":"2018-04-23T19:27:15Z","updated_at":"2018-09-24T17:20:17Z","due_on":null,"closed_at":null},"commits_url":"https://api.github.com/repos/highfidelity/hifi/pulls/14010/commits","review_comments_url":"https://api.github.com/repos/highfidelity/hifi/pulls/14010/comments","review_comment_url":"https://api.github.com/repos/highfidelity/hifi/pulls/comments{/number}","comments_url":"https://api.github.com/repos/highfidelity/hifi/issues/14010/comments","statuses_url":"https://api.github.com/repos/highfidelity/hifi/statuses/86e502637e581d6cdd9e5e6cc48cb37215891dd7","head":{"label":"sethalves:thread-safe-settings","ref":"thread-safe-settings","sha":"86e502637e581d6cdd9e5e6cc48cb37215891dd7","user":{"login":"sethalves","id":1137072,"node_id":"MDQ6VXNlcjExMzcwNzI=","avatar_url":"https://avatars1.githubusercontent.com/u/1137072?v=4","gravatar_id":"","url":"https://api.github.com/users/sethalves","html_url":"https://github.com/sethalves","followers_url":"https://api.github.com/users/sethalves/followers","following_url":"https://api.github.com/users/sethalves/following{/other_user}","gists_url":"https://api.github.com/users/sethalves/gists{/gist_id}","starred_url":"https://api.github.com/users/sethalves/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sethalves/subscriptions","organizations_url":"https://api.github.com/users/sethalves/orgs","repos_url":"https://api.github.com/users/sethalves/repos","events_url":"https://api.github.com/users/sethalves/events{/privacy}","received_events_url":"https://api.github.com/users/sethalves/received_events","type":"User","site_admin":false},"repo":{"id":30046830,"node_id":"MDEwOlJlcG9zaXRvcnkzMDA0NjgzMA==","name":"hifi","full_name":"sethalves/hifi","private":false,"owner":{"login":"sethalves","id":1137072,"node_id":"MDQ6VXNlcjExMzcwNzI=","avatar_url":"https://avatars1.githubusercontent.com/u/1137072?v=4","gravatar_id":"","url":"https://api.github.com/users/sethalves","html_url":"https://github.com/sethalves","followers_url":"https://api.github.com/users/sethalves/followers","following_url":"https://api.github.com/users/sethalves/following{/other_user}","gists_url":"https://api.github.com/users/sethalves/gists{/gist_id}","starred_url":"https://api.github.com/users/sethalves/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sethalves/subscriptions","organizations_url":"https://api.github.com/users/sethalves/orgs","repos_url":"https://api.github.com/users/sethalves/repos","events_url":"https://api.github.com/users/sethalves/events{/privacy}","received_events_url":"https://api.github.com/users/sethalves/received_events","type":"User","site_admin":false},"html_url":"https://github.com/sethalves/hifi","description":"Open, decentralized virtual worlds using sensors to control avatars and dynamically assigned devices as servers.  San Francisco based startup, we are hiring: http://highfidelity.io/jobs  Documentation at http://docs.highfidelity.io  Contribute by reporting bugs & doing jobs on http://worklist.net","fork":true,"url":"https://api.github.com/repos/sethalves/hifi","forks_url":"https://api.github.com/repos/sethalves/hifi/forks","keys_url":"https://api.github.com/repos/sethalves/hifi/keys{/key_id}","collaborators_url":"https://api.github.com/repos/sethalves/hifi/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/sethalves/hifi/teams","hooks_url":"https://api.github.com/repos/sethalves/hifi/hooks","issue_events_url":"https://api.github.com/repos/sethalves/hifi/issues/events{/number}","events_url":"https://api.github.com/repos/sethalves/hifi/events","assignees_url":"https://api.github.com/repos/sethalves/hifi/assignees{/user}","branches_url":"https://api.github.com/repos/sethalves/hifi/branches{/branch}","tags_url":"https://api.github.com/repos/sethalves/hifi/tags","blobs_url":"https://api.github.com/repos/sethalves/hifi/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/sethalves/hifi/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/sethalves/hifi/git/refs{/sha}","trees_url":"https://api.github.com/repos/sethalves/hifi/git/trees{/sha}","statuses_url":"https://api.github.com/repos/sethalves/hifi/statuses/{sha}","languages_url":"https://api.github.com/repos/sethalves/hifi/languages","stargazers_url":"https://api.github.com/repos/sethalves/hifi/stargazers","contributors_url":"https://api.github.com/repos/sethalves/hifi/contributors","subscribers_url":"https://api.github.com/repos/sethalves/hifi/subscribers","subscription_url":"https://api.github.com/repos/sethalves/hifi/subscription","commits_url":"https://api.github.com/repos/sethalves/hifi/commits{/sha}","git_commits_url":"https://api.github.com/repos/sethalves/hifi/git/commits{/sha}","comments_url":"https://api.github.com/repos/sethalves/hifi/comments{/number}","issue_comment_url":"https://api.github.com/repos/sethalves/hifi/issues/comments{/number}","contents_url":"https://api.github.com/repos/sethalves/hifi/contents/{+path}","compare_url":"https://api.github.com/repos/sethalves/hifi/compare/{base}...{head}","merges_url":"https://api.github.com/repos/sethalves/hifi/merges","archive_url":"https://api.github.com/repos/sethalves/hifi/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/sethalves/hifi/downloads","issues_url":"https://api.github.com/repos/sethalves/hifi/issues{/number}","pulls_url":"https://api.github.com/repos/sethalves/hifi/pulls{/number}","milestones_url":"https://api.github.com/repos/sethalves/hifi/milestones{/number}","notifications_url":"https://api.github.com/repos/sethalves/hifi/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/sethalves/hifi/labels{/name}","releases_url":"https://api.github.com/repos/sethalves/hifi/releases{/id}","deployments_url":"https://api.github.com/repos/sethalves/hifi/deployments","created_at":"2015-01-29T23:37:31Z","updated_at":"2018-09-16T17:14:12Z","pushed_at":"2018-09-23T22:09:36Z","git_url":"git://github.com/sethalves/hifi.git","ssh_url":"git@github.com:sethalves/hifi.git","clone_url":"https://github.com/sethalves/hifi.git","svn_url":"https://github.com/sethalves/hifi","homepage":"","size":665692,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":1,"license":{"key":"other","name":"Other","spdx_id":null,"url":null,"node_id":"MDc6TGljZW5zZTA="},"forks":0,"open_issues":1,"watchers":0,"default_branch":"master"}},"base":{"label":"highfidelity:master","ref":"master","sha":"ea7bc26ad8ca586ee74905625f662d37fd716919","user":{"login":"highfidelity","id":1565370,"node_id":"MDEyOk9yZ2FuaXphdGlvbjE1NjUzNzA=","avatar_url":"https://avatars0.githubusercontent.com/u/1565370?v=4","gravatar_id":"","url":"https://api.github.com/users/highfidelity","html_url":"https://github.com/highfidelity","followers_url":"https://api.github.com/users/highfidelity/followers","following_url":"https://api.github.com/users/highfidelity/following{/other_user}","gists_url":"https://api.github.com/users/highfidelity/gists{/gist_id}","starred_url":"https://api.github.com/users/highfidelity/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/highfidelity/subscriptions","organizations_url":"https://api.github.com/users/highfidelity/orgs","repos_url":"https://api.github.com/users/highfidelity/repos","events_url":"https://api.github.com/users/highfidelity/events{/privacy}","received_events_url":"https://api.github.com/users/highfidelity/received_events","type":"Organization","site_admin":false},"repo":{"id":5372872,"node_id":"MDEwOlJlcG9zaXRvcnk1MzcyODcy","name":"hifi","full_name":"highfidelity/hifi","private":false,"owner":{"login":"highfidelity","id":1565370,"node_id":"MDEyOk9yZ2FuaXphdGlvbjE1NjUzNzA=","avatar_url":"https://avatars0.githubusercontent.com/u/1565370?v=4","gravatar_id":"","url":"https://api.github.com/users/highfidelity","html_url":"https://github.com/highfidelity","followers_url":"https://api.github.com/users/highfidelity/followers","following_url":"https://api.github.com/users/highfidelity/following{/other_user}","gists_url":"https://api.github.com/users/highfidelity/gists{/gist_id}","starred_url":"https://api.github.com/users/highfidelity/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/highfidelity/subscriptions","organizations_url":"https://api.github.com/users/highfidelity/orgs","repos_url":"https://api.github.com/users/highfidelity/repos","events_url":"https://api.github.com/users/highfidelity/events{/privacy}","received_events_url":"https://api.github.com/users/highfidelity/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/highfidelity/hifi","description":"Open source client/server software for creating shared VR environments.  Docs at http://docs.highfidelity.com  We are hiring: http://highfidelity.com/jobs  Also, PT work available at http://worklist.net","fork":false,"url":"https://api.github.com/repos/highfidelity/hifi","forks_url":"https://api.github.com/repos/highfidelity/hifi/forks","keys_url":"https://api.github.com/repos/highfidelity/hifi/keys{/key_id}","collaborators_url":"https://api.github.com/repos/highfidelity/hifi/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/highfidelity/hifi/teams","hooks_url":"https://api.github.com/repos/highfidelity/hifi/hooks","issue_events_url":"https://api.github.com/repos/highfidelity/hifi/issues/events{/number}","events_url":"https://api.github.com/repos/highfidelity/hifi/events","assignees_url":"https://api.github.com/repos/highfidelity/hifi/assignees{/user}","branches_url":"https://api.github.com/repos/highfidelity/hifi/branches{/branch}","tags_url":"https://api.github.com/repos/highfidelity/hifi/tags","blobs_url":"https://api.github.com/repos/highfidelity/hifi/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/highfidelity/hifi/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/highfidelity/hifi/git/refs{/sha}","trees_url":"https://api.github.com/repos/highfidelity/hifi/git/trees{/sha}","statuses_url":"https://api.github.com/repos/highfidelity/hifi/statuses/{sha}","languages_url":"https://api.github.com/repos/highfidelity/hifi/languages","stargazers_url":"https://api.github.com/repos/highfidelity/hifi/stargazers","contributors_url":"https://api.github.com/repos/highfidelity/hifi/contributors","subscribers_url":"https://api.github.com/repos/highfidelity/hifi/subscribers","subscription_url":"https://api.github.com/repos/highfidelity/hifi/subscription","commits_url":"https://api.github.com/repos/highfidelity/hifi/commits{/sha}","git_commits_url":"https://api.github.com/repos/highfidelity/hifi/git/commits{/sha}","comments_url":"https://api.github.com/repos/highfidelity/hifi/comments{/number}","issue_comment_url":"https://api.github.com/repos/highfidelity/hifi/issues/comments{/number}","contents_url":"https://api.github.com/repos/highfidelity/hifi/contents/{+path}","compare_url":"https://api.github.com/repos/highfidelity/hifi/compare/{base}...{head}","merges_url":"https://api.github.com/repos/highfidelity/hifi/merges","archive_url":"https://api.github.com/repos/highfidelity/hifi/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/highfidelity/hifi/downloads","issues_url":"https://api.github.com/repos/highfidelity/hifi/issues{/number}","pulls_url":"https://api.github.com/repos/highfidelity/hifi/pulls{/number}","milestones_url":"https://api.github.com/repos/highfidelity/hifi/milestones{/number}","notifications_url":"https://api.github.com/repos/highfidelity/hifi/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/highfidelity/hifi/labels{/name}","releases_url":"https://api.github.com/repos/highfidelity/hifi/releases{/id}","deployments_url":"https://api.github.com/repos/highfidelity/hifi/deployments","created_at":"2012-08-10T18:26:22Z","updated_at":"2018-09-21T18:45:11Z","pushed_at":"2018-09-24T17:28:53Z","git_url":"git://github.com/highfidelity/hifi.git","ssh_url":"git@github.com:highfidelity/hifi.git","clone_url":"https://github.com/highfidelity/hifi.git","svn_url":"https://github.com/highfidelity/hifi","homepage":"","size":559060,"stargazers_count":643,"watchers_count":643,"language":"C++","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":415,"mirror_url":null,"archived":false,"open_issues_count":217,"license":{"key":"other","name":"Other","spdx_id":null,"url":null,"node_id":"MDc6TGljZW5zZTA="},"forks":415,"open_issues":217,"watchers":643,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/14010"},"html":{"href":"https://github.com/highfidelity/hifi/pull/14010"},"issue":{"href":"https://api.github.com/repos/highfidelity/hifi/issues/14010"},"comments":{"href":"https://api.github.com/repos/highfidelity/hifi/issues/14010/comments"},"review_comments":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/14010/comments"},"review_comment":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/highfidelity/hifi/pulls/14010/commits"},"statuses":{"href":"https://api.github.com/repos/highfidelity/hifi/statuses/86e502637e581d6cdd9e5e6cc48cb37215891dd7"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"Atlante45"}}