{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/2301","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/2301/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/2301/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/2301/events","html_url":"https://github.com/dotnet/coreclr/issues/2301","id":121599243,"number":2301,"title":"Race condition in DateTimeFormatInfo.DateTimeOffsetPattern","user":{"login":"ccurrens","id":1033555,"avatar_url":"https://avatars.githubusercontent.com/u/1033555?v=3","gravatar_id":"","url":"https://api.github.com/users/ccurrens","html_url":"https://github.com/ccurrens","followers_url":"https://api.github.com/users/ccurrens/followers","following_url":"https://api.github.com/users/ccurrens/following{/other_user}","gists_url":"https://api.github.com/users/ccurrens/gists{/gist_id}","starred_url":"https://api.github.com/users/ccurrens/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ccurrens/subscriptions","organizations_url":"https://api.github.com/users/ccurrens/orgs","repos_url":"https://api.github.com/users/ccurrens/repos","events_url":"https://api.github.com/users/ccurrens/events{/privacy}","received_events_url":"https://api.github.com/users/ccurrens/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-12-10T23:09:51Z","updated_at":"2015-12-11T21:59:15Z","closed_at":"2015-12-11T16:42:08Z","body":"There is a race condition that I've seen in some of our services in `DateTimeFormatInfo.DateTimeOffsetPattern`. I was seeing the failure when `DateTimeOffset.ToString()` was returning a string with two offsets, like `12/10/2015 22:13:23 +00:00 +00:00`. I originally noticed this in the full .NET Framework, but was able to reproduce it in CoreCLR, as well.\r\n\r\nUpon further investigation, it looks like the race condition can cause one of two formats:\r\n\r\n* `MM/dd/yyyy HH:mm:ss`\r\n* `MM/dd/yyyy HH:mm:ss zzz zzz`\r\n\r\nIn our production code that was occasionally hitting this bug, we weren't directly using `DateTimeFormatInfo`. We were seeing the issue when using a basic `string.Format` with a `DateTimeOffset` as one of the arguments.\r\n\r\nSince `CultureInfo` holds onto an instance of `DateTimeFormatInfo`, if your code executes just right you can have all `DateTimeOffset.ToString()` calls with incorrect formats for the life of your application. You can force the `DateTimeFormatInfo` to regenerate the format string calling the setter on the `LongTimePattern` or `ShortDatePattern` properties (although there's no guarantee you won't immediately hit the race condition again).\r\n\r\nI've [created a gist](https://gist.github.com/ccurrens/4d3a2670429a9e68064e) that can demonstrates the race condition.\r\n\r\nThe race is caused because the code is directly working against the `dateTimeOffsetPattern` member when building the pattern string. The pattern string is the concatenation of the short date and long time patterns. Some cultures include the offset in the long time pattern, so the code checks for that. In the case where the offset is not included in the string, it concatenates the offset format like so:\r\n\r\n    if (!foundZ) {\r\n        dateTimeOffsetPattern = dateTimeOffsetPattern + \" zzz\";\r\n    }\r\n\r\nDepending on execution order, sometimes this gets executed twice or executed in one thread and not in another.\r\n\r\nA non-intrusive fix would be to use a temporary variable to build pattern instead of directly assigning it to the class member. In the case where multiple threads are causing the property to be evaluated, they'll do duplicate work (as it currently does) but will each produce the same correct format string."},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/164061112","html_url":"https://github.com/dotnet/coreclr/issues/2301#issuecomment-164061112","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/2301","id":164061112,"user":{"login":"stephentoub","id":2642209,"avatar_url":"https://avatars.githubusercontent.com/u/2642209?v=3","gravatar_id":"","url":"https://api.github.com/users/stephentoub","html_url":"https://github.com/stephentoub","followers_url":"https://api.github.com/users/stephentoub/followers","following_url":"https://api.github.com/users/stephentoub/following{/other_user}","gists_url":"https://api.github.com/users/stephentoub/gists{/gist_id}","starred_url":"https://api.github.com/users/stephentoub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stephentoub/subscriptions","organizations_url":"https://api.github.com/users/stephentoub/orgs","repos_url":"https://api.github.com/users/stephentoub/repos","events_url":"https://api.github.com/users/stephentoub/events{/privacy}","received_events_url":"https://api.github.com/users/stephentoub/received_events","type":"User","site_admin":false},"created_at":"2015-12-11T21:59:15Z","updated_at":"2015-12-11T21:59:15Z","body":"> Yeah, it appears to be\r\n\r\nI should clarify my comments.  It's possible the docs are in fact correct, in which case we potentially have bugs to fix.\r\n\r\nAccessing members of a CultureInfo retrieved from a public static propery that publishes that instance to multiple threads needs to be safe to do from multiple threads concurrently... if it already is (aside from the random bug like the one highlighted in this issue), then we should update the docs (and fix any other bugs we might find). And if the docs are correct and there are actually bugs here, then we should find and fix those in the source.\r\n\r\nIt may also be that the runtime only publishes certain instances (e.g. like a singleton for invariant culture), in which case it may be that the docs are correct in general but should have a caveat about these certain cases.  And we should ensure that for those specific cases it actually is safe in the implementation."}}