{"thread":{"id":"28323","subject":"[RFC/PATCH] core.clientTimeout config for git:// fetches","startedAt":"2011-09-07T10:15:45Z","lastAt":"2011-09-07T10:15:45Z","messageCount":1,"participants":["Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174984","messageId":"20110907101545.GA9326@dcvr.yhbt.net","threadId":"28323","inReplyTo":null,"subject":"[RFC/PATCH] core.clientTimeout config for git:// fetches","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2011-09-07T10:15:45Z","receivedAt":"2011-09-07T10:15:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This allows \"git fetch\" to exit after a preconfigured socket\ntimeout.  This is needed in case an undetected server or network\nfailure occurs.\n\nThe default timeout is 60 seconds.\n\nhttp:// and rsync:// transports should be supported in future\nchanges, too.\n\nSo far, I've only tested (by hand) with a dummy server setup\nusing \"nc -l $some_port\".\n---\n\n  I run \"git fetch\" from cron to keep mirrors up-to-date.  I've\n  noticed some cron processes running for hours/days at a time after\n  network failures.  When I straced thoses processes, I saw they were\n  stuck on read() syscalls to remote machines.\n\n  I'm not familiar with this code and hope there's a better way to do\n  this.  Changing wrapper.c like this in this way doesn't feel right,\n  but neither does sprinkling alarm() calls in various places.\n\n  Using a custom GIT_PROXY_COMMAND which implements timeouts on its own\n  would also work, but I'd like to eventually have a consistent parameter\n  unified across all remote transports for consistency and ease-of-use.\n\n  The default 60s timeout could be too low on some slow servers\n  with big repos, maybe the default should remain zero (no timeout).\n\n connect.c |   27 +++++++++++++++++++++++++++\n wrapper.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 72 insertions(+), 2 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex ee1d4b4..c1fa1ab 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -320,10 +320,37 @@ static int git_tcp_connect_sock(char *host, int flags)\n \n #endif /* NO_IPV6 */\n \n+static int timeout_setup(const char *var, const char *value, void *cb)\n+{\n+\tint *timeout = cb;\n+\n+\tif (strcmp(var, \"core.clienttimeout\") == 0)\n+\t\t*timeout = git_config_int(var, value);\n+\n+\treturn 0;\n+}\n \n static void git_tcp_connect(int fd[2], char *host, int flags)\n {\n \tint sockfd = git_tcp_connect_sock(host, flags);\n+\tstruct timeval tv;\n+\tint timeout = 60;\n+\n+\tgit_config(timeout_setup, &timeout);\n+\n+\tif (timeout > 0) {\n+\t\tsocklen_t len = sizeof(struct timeval);\n+\n+\t\ttv.tv_sec = timeout;\n+\t\ttv.tv_usec = 0;\n+\t\tif (setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &tv, len) < 0)\n+\t\t\tdie_errno(\"could not set socket receive timeout\");\n+\n+\t\ttv.tv_sec = timeout;\n+\t\ttv.tv_usec = 0;\n+\t\tif (setsockopt(sockfd, SOL_SOCKET, SO_SNDTIMEO, &tv, len) < 0)\n+\t\t\tdie_errno(\"could not set socket send timeout\");\n+\t}\n \n \tfd[0] = sockfd;\n \tfd[1] = dup(sockfd);\ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..09a31c6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -109,6 +109,49 @@ void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n+static int io_continuable(int fd, int optname)\n+{\n+\tif (errno == EINTR)\n+\t\treturn 1;\n+\n+\t/*\n+\t * If fd is a socket, a socket-level timeout could've been\n+\t * set in git_tcp_connect().  Unfortunately EAGAIN alone\n+\t * isn't enough to tell us if the timeout is user-specified\n+\t * or not, so we have to try to figure it out.\n+\t */\n+\tif (errno == EAGAIN) {\n+\t\tsocklen_t len = sizeof(struct timeval);\n+\t\tstruct timeval tv;\n+\t\tint flags;\n+\n+\t\t/*\n+\t\t * Let any non-blocking I/O users through.  Currently\n+\t\t * git does not explicitly use non-blocking I/O anywhere,\n+\t\t * but it could inherit a non-blocking descriptor.  We\n+\t\t * rely on the fact that (currently) no sockets git itself\n+\t\t * creates (and thus may set timeout on) also sets O_NONBLOCK.\n+\t\t */\n+\t\tflags = fcntl(fd, F_GETFL);\n+\t\tif (flags < 0)\n+\t\t\tdie_errno(\"fcntl failed to get file status flags\");\n+\t\tif (flags & O_NONBLOCK)\n+\t\t\treturn 1;\n+\n+\t\tif (getsockopt(fd, SOL_SOCKET, optname, &tv, &len) < 0) {\n+\t\t\tif (errno == ENOTSOCK)\n+\t\t\t\treturn 1;\n+\t\t\tdie_errno(\"getsockopt failed to get socket option\");\n+\t\t}\n+\t\tif (tv.tv_sec > 0 || tv.tv_usec > 0)\n+\t\t\tdie(\"socket timed out\");\n+\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n /*\n  * xread() is the same a read(), but it automatically restarts read()\n  * operations with a recoverable error (EAGAIN and EINTR). xread()\n@@ -119,7 +162,7 @@ ssize_t xread(int fd, void *buf, size_t len)\n \tssize_t nr;\n \twhile (1) {\n \t\tnr = read(fd, buf, len);\n-\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\tif (nr < 0 && io_continuable(fd, SO_RCVTIMEO))\n \t\t\tcontinue;\n \t\treturn nr;\n \t}\n@@ -135,7 +178,7 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \tssize_t nr;\n \twhile (1) {\n \t\tnr = write(fd, buf, len);\n-\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\tif (nr < 0 && io_continuable(fd, SO_SNDTIMEO))\n \t\t\tcontinue;\n \t\treturn nr;\n \t}\n-- \nEric Wong\n"}]}