{"action":"created","issue":{"url":"https://api.github.com/repos/typhoeus/ethon/issues/105","labels_url":"https://api.github.com/repos/typhoeus/ethon/issues/105/labels{/name}","comments_url":"https://api.github.com/repos/typhoeus/ethon/issues/105/comments","events_url":"https://api.github.com/repos/typhoeus/ethon/issues/105/events","html_url":"https://github.com/typhoeus/ethon/pull/105","id":98846643,"number":105,"title":"redo select on EINTR","user":{"login":"thwarted","id":35268,"avatar_url":"https://avatars.githubusercontent.com/u/35268?v=3","gravatar_id":"","url":"https://api.github.com/users/thwarted","html_url":"https://github.com/thwarted","followers_url":"https://api.github.com/users/thwarted/followers","following_url":"https://api.github.com/users/thwarted/following{/other_user}","gists_url":"https://api.github.com/users/thwarted/gists{/gist_id}","starred_url":"https://api.github.com/users/thwarted/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thwarted/subscriptions","organizations_url":"https://api.github.com/users/thwarted/orgs","repos_url":"https://api.github.com/users/thwarted/repos","events_url":"https://api.github.com/users/thwarted/events{/privacy}","received_events_url":"https://api.github.com/users/thwarted/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-08-03T22:00:04Z","updated_at":"2015-08-08T01:42:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/typhoeus/ethon/pulls/105","html_url":"https://github.com/typhoeus/ethon/pull/105","diff_url":"https://github.com/typhoeus/ethon/pull/105.diff","patch_url":"https://github.com/typhoeus/ethon/pull/105.patch"},"body":"Attempt to resolve typhoeus/ethon issue #31.\r\n\r\nRedo the select when it returns EINTR (errno==4), indicating that it has\r\nbeen interrupted by the exection of a signal handler.  No attempt is\r\nmade to set a new timeout since that is non-portable (and straces\r\nindicates some fractional second timeout values being passed). This is\r\nintended to be, at worst, waiting up to twice as long as the timeout\r\ngiven in the event of a signal, which is argubly better than fully\r\naborting on receiving a signal that has a signal handler installed.\r\n\r\nIt makes more sense to do this in ethon than in the calling code because\r\nmultiple libcurl interfaces are being used within the set_fds method:\r\nboth Curl.multi_fdset and Curl.select, and the reinvocation of select\r\nshould have the same fdsets, which wouldn't strictly be guaranteed by\r\ncalling the set_fds method again.\r\n\r\nHowever, it may make the most sense to do this in libcurl.  Where the\r\nline should be drawn is kind of hazy. A quick search yielded no libcurl\r\nexamples that do anything other than emit the errno value back to the\r\ncaller.\r\n\r\nScript to reproduce are at https://gist.github.com/thwarted/76a16b2d208ffe2ac223"},"comment":{"url":"https://api.github.com/repos/typhoeus/ethon/issues/comments/128880364","html_url":"https://github.com/typhoeus/ethon/pull/105#issuecomment-128880364","issue_url":"https://api.github.com/repos/typhoeus/ethon/issues/105","id":128880364,"user":{"login":"thwarted","id":35268,"avatar_url":"https://avatars.githubusercontent.com/u/35268?v=3","gravatar_id":"","url":"https://api.github.com/users/thwarted","html_url":"https://github.com/thwarted","followers_url":"https://api.github.com/users/thwarted/followers","following_url":"https://api.github.com/users/thwarted/following{/other_user}","gists_url":"https://api.github.com/users/thwarted/gists{/gist_id}","starred_url":"https://api.github.com/users/thwarted/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thwarted/subscriptions","organizations_url":"https://api.github.com/users/thwarted/orgs","repos_url":"https://api.github.com/users/thwarted/repos","events_url":"https://api.github.com/users/thwarted/events{/privacy}","received_events_url":"https://api.github.com/users/thwarted/received_events","type":"User","site_admin":false},"created_at":"2015-08-08T01:42:40Z","updated_at":"2015-08-08T01:42:40Z","body":"I'm not confident the value of `::FFI.errno` is valid unless `code < 0`, so the condition on `code` needs to be there.  Physically it's two conditions, but logically they are one check because of the way errors are emitted from system calls.  If you want it to be an if, it would have to be\r\n```\r\nbreak if code >= 0 || ::FFI.errno != Errno::EINTR::Errno\r\n```\r\nhowever I find this harder to read because when it's `&&`, the first condition is a guard, in reading comprehension, against the second. I also don't think it communicates the intent as well, since `code` is the return value from the system call, and `< 0` is a common idiom when checking for an error state in the return value from system calls.\r\n\r\nAnother option is\r\n```\r\nbreak if !(code < 0 && ::FFI.errno == Errno::EINTR::Errno)\r\n```\r\nwhich is semantically equivalent to the `unless` wording, but, IMO, has higher cognitive load.\r\n\r\nIt seems that `Curl.select` is mostly an alias for `Kernel#select` (or an alias for libc's select, I'm not familiar enough with ruby's FFI to tell), as shown in [line 59 of functions.rb](https://github.com/typhoeus/ethon/blob/HEAD/lib/ethon/curls/functions.rb#L59).  I also can't find any reference to a curl-specific select function in libcurl, so this makes sense.  The [docs for `multi_fdset` also reference `select(2)`](http://curl.haxx.se/libcurl/c/curl_multi_fdset.html).  In light of this, there are no \"curl function invocations\" between multiple calls to `Curl.select` in this loop, so that note from the [libcurl tutorial](http://curl.haxx.se/libcurl/c/libcurl-tutorial.html) doesn't apply.\r\n\r\nI initially thought that `code` should be compared against `:ok` as the return value is from [`Curl.multi_fdset` on line 120](https://github.com/typhoeus/ethon/pull/105/files#diff-52c8b8dce6b68c8b45f48eca2ab8c28cL120), but since `Curl.select` isn't a libcurl function, the return value doesn't have libcurl return value semantics.\r\n\r\n"}}