{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/roslyn/issues/4001","labels_url":"https://api.github.com/repos/dotnet/roslyn/issues/4001/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/roslyn/issues/4001/comments","events_url":"https://api.github.com/repos/dotnet/roslyn/issues/4001/events","html_url":"https://github.com/dotnet/roslyn/pull/4001","id":95775549,"number":4001,"title":"Remove race conditions in ProducerPopulatedTagSource","user":{"login":"CyrusNajmabadi","id":4564579,"avatar_url":"https://avatars.githubusercontent.com/u/4564579?v=3","gravatar_id":"","url":"https://api.github.com/users/CyrusNajmabadi","html_url":"https://github.com/CyrusNajmabadi","followers_url":"https://api.github.com/users/CyrusNajmabadi/followers","following_url":"https://api.github.com/users/CyrusNajmabadi/following{/other_user}","gists_url":"https://api.github.com/users/CyrusNajmabadi/gists{/gist_id}","starred_url":"https://api.github.com/users/CyrusNajmabadi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CyrusNajmabadi/subscriptions","organizations_url":"https://api.github.com/users/CyrusNajmabadi/orgs","repos_url":"https://api.github.com/users/CyrusNajmabadi/repos","events_url":"https://api.github.com/users/CyrusNajmabadi/events{/privacy}","received_events_url":"https://api.github.com/users/CyrusNajmabadi/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dotnet/roslyn/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-07-18T01:38:07Z","updated_at":"2015-07-27T19:56:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/roslyn/pulls/4001","html_url":"https://github.com/dotnet/roslyn/pull/4001","diff_url":"https://github.com/dotnet/roslyn/pull/4001.diff","patch_url":"https://github.com/dotnet/roslyn/pull/4001.patch"},"body":"Previously ProducerPopulatedTagSource would compute results on the background and then mutate its internal state with those values it had computed.\r\n\r\nThen on the foreground it might also try to mutate those values.  This was very racey, and there were comments indicating as such in the code.  This was especially egregious for the code trying to track what span of text had been edited over time.  It was not only racey in how it accumulated and used this value, it was also racing in how it reset the value once it had been used.  This could mean we might think there was no text change when there actually was, or vice-versa.\r\n\r\nI've now switched the ProducerPopulatedTagSource  to strictly follow the pattern of\r\n\r\n1. On change, switch to foreground. (we were already doing this).  Snapshot state and enqueue bg request.\r\n2. On background compute new results.  Make no mutations to existing state.  Enqueue fg to update state.\r\n3. On foreground update state.\r\n\r\nThis removes the need for any sort of locking in this type, and makes the conceptual model much simpler.  We use the FG thread effectively as the serialization mechanism to make sure all reads/writes to this state are consistent and coherent.\r\n\r\nNote that even though we switch to the foreground at the end, all we're doing is updating our internal state, and then letting the BatchChangeNotifier know about the changes at that point.  The batch change notifier will still throttle and coalesce UI notifications to the editor about which spans have actually changed.  So it's not like this change will cause us to do undue stuff like calling into the editor too often with too many changes."},"comment":{"url":"https://api.github.com/repos/dotnet/roslyn/issues/comments/125325926","html_url":"https://github.com/dotnet/roslyn/pull/4001#issuecomment-125325926","issue_url":"https://api.github.com/repos/dotnet/roslyn/issues/4001","id":125325926,"user":{"login":"CyrusNajmabadi","id":4564579,"avatar_url":"https://avatars.githubusercontent.com/u/4564579?v=3","gravatar_id":"","url":"https://api.github.com/users/CyrusNajmabadi","html_url":"https://github.com/CyrusNajmabadi","followers_url":"https://api.github.com/users/CyrusNajmabadi/followers","following_url":"https://api.github.com/users/CyrusNajmabadi/following{/other_user}","gists_url":"https://api.github.com/users/CyrusNajmabadi/gists{/gist_id}","starred_url":"https://api.github.com/users/CyrusNajmabadi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CyrusNajmabadi/subscriptions","organizations_url":"https://api.github.com/users/CyrusNajmabadi/orgs","repos_url":"https://api.github.com/users/CyrusNajmabadi/repos","events_url":"https://api.github.com/users/CyrusNajmabadi/events{/privacy}","received_events_url":"https://api.github.com/users/CyrusNajmabadi/received_events","type":"User","site_admin":false},"created_at":"2015-07-27T19:56:44Z","updated_at":"2015-07-27T19:56:44Z","body":"I’m curious what’s wrong with using the FG thread.  We already report back to that thread for multiple purposes (i.e. the batch change notifier as well as the event source notifications).\r\n\r\nSince all work starts from the FG thread and ends back up on the FG thread, it seems like a nice place to do the state updating.\r\n\r\n             -- Cyrus\r\n\r\nFrom: Heejae Chang [mailto:notifications@github.com]\r\nSent: Monday, July 27, 2015 11:16 AM\r\nTo: dotnet/roslyn <roslyn@noreply.github.com>\r\nCc: Cyrus Najmabadi <cyrusn@microsoft.com>\r\nSubject: Re: [roslyn] Remove race conditions in ProducerPopulatedTagSource (#4001)\r\n\r\n\r\n@CyrusNajmabadi<https://github.com/CyrusNajmabadi> I would prefer either using lock or having another queue for serialization rather than using FG for such purpose.\r\n\r\n—\r\nReply to this email directly or view it on GitHub<https://github.com/dotnet/roslyn/pull/4001#issuecomment-125293278>.\r\n"}}