{"action":"created","issue":{"url":"https://api.github.com/repos/libuv/libuv/issues/349","labels_url":"https://api.github.com/repos/libuv/libuv/issues/349/labels{/name}","comments_url":"https://api.github.com/repos/libuv/libuv/issues/349/comments","events_url":"https://api.github.com/repos/libuv/libuv/issues/349/events","html_url":"https://github.com/libuv/libuv/pull/349","id":75388653,"number":349,"title":"unix: removed double-checked read from uv_async","user":{"login":"mpenick","id":3710715,"avatar_url":"https://avatars.githubusercontent.com/u/3710715?v=3","gravatar_id":"","url":"https://api.github.com/users/mpenick","html_url":"https://github.com/mpenick","followers_url":"https://api.github.com/users/mpenick/followers","following_url":"https://api.github.com/users/mpenick/following{/other_user}","gists_url":"https://api.github.com/users/mpenick/gists{/gist_id}","starred_url":"https://api.github.com/users/mpenick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mpenick/subscriptions","organizations_url":"https://api.github.com/users/mpenick/orgs","repos_url":"https://api.github.com/users/mpenick/repos","events_url":"https://api.github.com/users/mpenick/events{/privacy}","received_events_url":"https://api.github.com/users/mpenick/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-05-11T23:09:08Z","updated_at":"2015-05-12T04:05:01Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/libuv/libuv/pulls/349","html_url":"https://github.com/libuv/libuv/pull/349","diff_url":"https://github.com/libuv/libuv/pull/349.diff","patch_url":"https://github.com/libuv/libuv/pull/349.patch"},"body":"This provides only a negligible increase in performance, but can cause\r\nincorrect results. The ACCESS_ONCE() macro only provides a weak compiler\r\nbarrier on GCC. The problem is this \"handle->pending\" load can be\r\nreordered before a store that occurs in the client application causing\r\nthe write to be missed on the consuming side of uv_async_send(). From the\r\nIntel manual: \"Loads May Be Reordered with Earlier Stores to Different\r\nLocations\". This is also likely to cause a similar issue on other\r\narchitectures that are not as strictly ordered as x86."},"comment":{"url":"https://api.github.com/repos/libuv/libuv/issues/comments/101122729","html_url":"https://github.com/libuv/libuv/pull/349#issuecomment-101122729","issue_url":"https://api.github.com/repos/libuv/libuv/issues/349","id":101122729,"user":{"login":"mpenick","id":3710715,"avatar_url":"https://avatars.githubusercontent.com/u/3710715?v=3","gravatar_id":"","url":"https://api.github.com/users/mpenick","html_url":"https://github.com/mpenick","followers_url":"https://api.github.com/users/mpenick/followers","following_url":"https://api.github.com/users/mpenick/following{/other_user}","gists_url":"https://api.github.com/users/mpenick/gists{/gist_id}","starred_url":"https://api.github.com/users/mpenick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mpenick/subscriptions","organizations_url":"https://api.github.com/users/mpenick/orgs","repos_url":"https://api.github.com/users/mpenick/repos","events_url":"https://api.github.com/users/mpenick/events{/privacy}","received_events_url":"https://api.github.com/users/mpenick/received_events","type":"User","site_admin":false},"created_at":"2015-05-12T04:05:00Z","updated_at":"2015-05-12T04:05:00Z","body":"I wanted to expand on the the example I outlined before. There isn't a great way to easily show or explain multithreaded code so bear with me. :) Let me know if you want me to explain anything further.\r\n\r\nThe following code is being run over and over:\r\n\r\n```c\r\n/* ... */\r\n\r\nfor (...) {\r\n  /* Store some variable we want to see in the async callback */\r\n  atomic_store_explict(&some_var, some_value, memory_order_release);\r\n\r\n  uv_async_send(&async);\r\n\r\n  /* Step 3. `some_var` is finally stored by the processor */\r\n}\r\n\r\n/* ... */\r\n```\r\n\r\nNow imagine that we enter both `uv_async_send()` and `uv__async_event()` at the same time and `handle->pending` is already set (from a previous async send call). **Important**: The store to `some_var` hasn't happened yet because there is no total order between load and store to different locations, that is, the code doesn't have anything to explictitly enforce the order between the store of `some_var` and the load of `handle->pending`.\r\n\r\n```c\r\nint uv_async_send(uv_async_t* handle) {\r\n  /* Step 1. Load `handle->pending`, OR load, compare `handle->pending`,\r\n   * OR load, compared `handle->pending, jump, ..., etc. It's possible that all or none\r\n   * of this executes before the store to `some_var`.\r\n   */\r\n  if (ACCESS_ONCE(int, handle->pending) != 0)\r\n    return 0;\r\n\r\n  /* ... */\r\n}\r\n```\r\n\r\n\r\nMeanwhile in `uv__async_event()` is called on another thread:\r\n\r\n```c\r\nstatic void uv__async_event(uv_loop_t* loop,\r\n                            struct uv__async* w,\r\n                            unsigned int nevents) {\r\n  QUEUE* q;\r\n  uv_async_t* h;\r\n\r\n  QUEUE_FOREACH(q, &loop->async_handles) {\r\n    h = QUEUE_DATA(q, uv_async_t, queue);\r\n\r\n    /* Step 2. After the load of `handle->pending` (which is set) in `uv_async_send()` we\r\n     * compare and exchange here and run the callback, but remember that the store to\r\n     * `some_var` hasn't happened yet.\r\n     */\r\n    if (cmpxchgi(&h->pending, 1, 0) == 0)\r\n      continue;\r\n\r\n    if (h->async_cb == NULL)\r\n      continue;\r\n    h->async_cb(h);\r\n  }\r\n}\r\n```\r\n\r\nIn this scenario, the callback expected to see `some_var` with the value `some_value`, but it never happened because of the reordering of the store of `some_var` and the load of `handle->pending`. However, if that double-check is removed the compare exchange properly enforces the store of `some_var` before loading the `handle->pending` flag and properly signals the callback after the store.\r\n"}}