{"thread":{"id":"43041","subject":"[PATCH] http-backend: buffer headers before sending","startedAt":"2016-08-09T23:47:37Z","lastAt":"2016-08-10T21:20:40Z","messageCount":4,"participants":["Eric Wong","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"293552","messageId":"20160809234731.GA10310@dcvr","threadId":"43041","inReplyTo":null,"subject":"[PATCH] http-backend: buffer headers before sending","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-09T23:47:31Z","receivedAt":"2016-08-09T23:47:37Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Avoid waking up the readers for unnecessary context switches for\neach line of header data being written, as all the headers are\nwritten in short succession.\n\nIt is unlikely any HTTP/1.x server would want to read a CGI\nresponse one-line-at-a-time and trickle each to the client.\nInstead, I'd expect HTTP servers want to minimize syscall and\nTCP/IP framing overhead by trying to send all of its response\nheaders in a single syscall or even combining the headers and\nfirst chunk of the body with MSG_MORE or writev.\n\nVerified by strace-ing response parsing on the CGI side.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n  I admit I only noticed this because I was being lazy when\n  implementing the reader-side on an HTTP server by making\n  a single read(2) call :x\n\n http-backend.c | 220 ++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 116 insertions(+), 104 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 0d59499..adc8c8c 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -75,55 +75,57 @@ static void format_write(int fd, const char *fmt, ...)\n \twrite_or_die(fd, buffer, n);\n }\n \n-static void http_status(unsigned code, const char *msg)\n+static void http_status(struct strbuf *hdr, unsigned code, const char *msg)\n {\n-\tformat_write(1, \"Status: %u %s\\r\\n\", code, msg);\n+\tstrbuf_addf(hdr, \"Status: %u %s\\r\\n\", code, msg);\n }\n \n-static void hdr_str(const char *name, const char *value)\n+static void hdr_str(struct strbuf *hdr, const char *name, const char *value)\n {\n-\tformat_write(1, \"%s: %s\\r\\n\", name, value);\n+\tstrbuf_addf(hdr, \"%s: %s\\r\\n\", name, value);\n }\n \n-static void hdr_int(const char *name, uintmax_t value)\n+static void hdr_int(struct strbuf *hdr, const char *name, uintmax_t value)\n {\n-\tformat_write(1, \"%s: %\" PRIuMAX \"\\r\\n\", name, value);\n+\tstrbuf_addf(hdr, \"%s: %\" PRIuMAX \"\\r\\n\", name, value);\n }\n \n-static void hdr_date(const char *name, unsigned long when)\n+static void hdr_date(struct strbuf *hdr, const char *name, unsigned long when)\n {\n \tconst char *value = show_date(when, 0, DATE_MODE(RFC2822));\n-\thdr_str(name, value);\n+\thdr_str(hdr, name, value);\n }\n \n-static void hdr_nocache(void)\n+static void hdr_nocache(struct strbuf *hdr)\n {\n-\thdr_str(\"Expires\", \"Fri, 01 Jan 1980 00:00:00 GMT\");\n-\thdr_str(\"Pragma\", \"no-cache\");\n-\thdr_str(\"Cache-Control\", \"no-cache, max-age=0, must-revalidate\");\n+\thdr_str(hdr, \"Expires\", \"Fri, 01 Jan 1980 00:00:00 GMT\");\n+\thdr_str(hdr, \"Pragma\", \"no-cache\");\n+\thdr_str(hdr, \"Cache-Control\", \"no-cache, max-age=0, must-revalidate\");\n }\n \n-static void hdr_cache_forever(void)\n+static void hdr_cache_forever(struct strbuf *hdr)\n {\n \tunsigned long now = time(NULL);\n-\thdr_date(\"Date\", now);\n-\thdr_date(\"Expires\", now + 31536000);\n-\thdr_str(\"Cache-Control\", \"public, max-age=31536000\");\n+\thdr_date(hdr, \"Date\", now);\n+\thdr_date(hdr, \"Expires\", now + 31536000);\n+\thdr_str(hdr, \"Cache-Control\", \"public, max-age=31536000\");\n }\n \n-static void end_headers(void)\n+static void end_headers(struct strbuf *hdr)\n {\n-\twrite_or_die(1, \"\\r\\n\", 2);\n+\tstrbuf_add(hdr, \"\\r\\n\", 2);\n+\twrite_or_die(1, hdr->buf, hdr->len);\n+\tstrbuf_release(hdr);\n }\n \n-__attribute__((format (printf, 1, 2)))\n-static NORETURN void not_found(const char *err, ...)\n+__attribute__((format (printf, 2, 3)))\n+static NORETURN void not_found(struct strbuf *hdr, const char *err, ...)\n {\n \tva_list params;\n \n-\thttp_status(404, \"Not Found\");\n-\thdr_nocache();\n-\tend_headers();\n+\thttp_status(hdr, 404, \"Not Found\");\n+\thdr_nocache(hdr);\n+\tend_headers(hdr);\n \n \tva_start(params, err);\n \tif (err && *err)\n@@ -132,14 +134,14 @@ static NORETURN void not_found(const char *err, ...)\n \texit(0);\n }\n \n-__attribute__((format (printf, 1, 2)))\n-static NORETURN void forbidden(const char *err, ...)\n+__attribute__((format (printf, 2, 3)))\n+static NORETURN void forbidden(struct strbuf *hdr, const char *err, ...)\n {\n \tva_list params;\n \n-\thttp_status(403, \"Forbidden\");\n-\thdr_nocache();\n-\tend_headers();\n+\thttp_status(hdr, 403, \"Forbidden\");\n+\thdr_nocache(hdr);\n+\tend_headers(hdr);\n \n \tva_start(params, err);\n \tif (err && *err)\n@@ -148,21 +150,23 @@ static NORETURN void forbidden(const char *err, ...)\n \texit(0);\n }\n \n-static void select_getanyfile(void)\n+static void select_getanyfile(struct strbuf *hdr)\n {\n \tif (!getanyfile)\n-\t\tforbidden(\"Unsupported service: getanyfile\");\n+\t\tforbidden(hdr, \"Unsupported service: getanyfile\");\n }\n \n-static void send_strbuf(const char *type, struct strbuf *buf)\n+static void send_strbuf(struct strbuf *hdr,\n+\t\t\tconst char *type, struct strbuf *buf)\n {\n-\thdr_int(content_length, buf->len);\n-\thdr_str(content_type, type);\n-\tend_headers();\n+\thdr_int(hdr, content_length, buf->len);\n+\thdr_str(hdr, content_type, type);\n+\tend_headers(hdr);\n \twrite_or_die(1, buf->buf, buf->len);\n }\n \n-static void send_local_file(const char *the_type, const char *name)\n+static void send_local_file(struct strbuf *hdr, const char *the_type,\n+\t\t\t\tconst char *name)\n {\n \tchar *p = git_pathdup(\"%s\", name);\n \tsize_t buf_alloc = 8192;\n@@ -172,14 +176,14 @@ static void send_local_file(const char *the_type, const char *name)\n \n \tfd = open(p, O_RDONLY);\n \tif (fd < 0)\n-\t\tnot_found(\"Cannot open '%s': %s\", p, strerror(errno));\n+\t\tnot_found(hdr, \"Cannot open '%s': %s\", p, strerror(errno));\n \tif (fstat(fd, &sb) < 0)\n \t\tdie_errno(\"Cannot stat '%s'\", p);\n \n-\thdr_int(content_length, sb.st_size);\n-\thdr_str(content_type, the_type);\n-\thdr_date(last_modified, sb.st_mtime);\n-\tend_headers();\n+\thdr_int(hdr, content_length, sb.st_size);\n+\thdr_str(hdr, content_type, the_type);\n+\thdr_date(hdr, last_modified, sb.st_mtime);\n+\tend_headers(hdr);\n \n \tfor (;;) {\n \t\tssize_t n = xread(fd, buf, buf_alloc);\n@@ -194,32 +198,32 @@ static void send_local_file(const char *the_type, const char *name)\n \tfree(p);\n }\n \n-static void get_text_file(char *name)\n+static void get_text_file(struct strbuf *hdr, char *name)\n {\n-\tselect_getanyfile();\n-\thdr_nocache();\n-\tsend_local_file(\"text/plain\", name);\n+\tselect_getanyfile(hdr);\n+\thdr_nocache(hdr);\n+\tsend_local_file(hdr, \"text/plain\", name);\n }\n \n-static void get_loose_object(char *name)\n+static void get_loose_object(struct strbuf *hdr, char *name)\n {\n-\tselect_getanyfile();\n-\thdr_cache_forever();\n-\tsend_local_file(\"application/x-git-loose-object\", name);\n+\tselect_getanyfile(hdr);\n+\thdr_cache_forever(hdr);\n+\tsend_local_file(hdr, \"application/x-git-loose-object\", name);\n }\n \n-static void get_pack_file(char *name)\n+static void get_pack_file(struct strbuf *hdr, char *name)\n {\n-\tselect_getanyfile();\n-\thdr_cache_forever();\n-\tsend_local_file(\"application/x-git-packed-objects\", name);\n+\tselect_getanyfile(hdr);\n+\thdr_cache_forever(hdr);\n+\tsend_local_file(hdr, \"application/x-git-packed-objects\", name);\n }\n \n-static void get_idx_file(char *name)\n+static void get_idx_file(struct strbuf *hdr, char *name)\n {\n-\tselect_getanyfile();\n-\thdr_cache_forever();\n-\tsend_local_file(\"application/x-git-packed-objects-toc\", name);\n+\tselect_getanyfile(hdr);\n+\thdr_cache_forever(hdr);\n+\tsend_local_file(hdr, \"application/x-git-packed-objects-toc\", name);\n }\n \n static void http_config(void)\n@@ -241,14 +245,14 @@ static void http_config(void)\n \tstrbuf_release(&var);\n }\n \n-static struct rpc_service *select_service(const char *name)\n+static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n {\n \tconst char *svc_name;\n \tstruct rpc_service *svc = NULL;\n \tint i;\n \n \tif (!skip_prefix(name, \"git-\", &svc_name))\n-\t\tforbidden(\"Unsupported service: '%s'\", name);\n+\t\tforbidden(hdr, \"Unsupported service: '%s'\", name);\n \n \tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n \t\tstruct rpc_service *s = &rpc_service[i];\n@@ -259,14 +263,14 @@ static struct rpc_service *select_service(const char *name)\n \t}\n \n \tif (!svc)\n-\t\tforbidden(\"Unsupported service: '%s'\", name);\n+\t\tforbidden(hdr, \"Unsupported service: '%s'\", name);\n \n \tif (svc->enabled < 0) {\n \t\tconst char *user = getenv(\"REMOTE_USER\");\n \t\tsvc->enabled = (user && *user) ? 1 : 0;\n \t}\n \tif (!svc->enabled)\n-\t\tforbidden(\"Service not enabled: '%s'\", svc->name);\n+\t\tforbidden(hdr, \"Service not enabled: '%s'\", svc->name);\n \treturn svc;\n }\n \n@@ -442,23 +446,23 @@ static int show_text_ref(const char *name, const struct object_id *oid,\n \treturn 0;\n }\n \n-static void get_info_refs(char *arg)\n+static void get_info_refs(struct strbuf *hdr, char *arg)\n {\n \tconst char *service_name = get_parameter(\"service\");\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\thdr_nocache();\n+\thdr_nocache(hdr);\n \n \tif (service_name) {\n \t\tconst char *argv[] = {NULL /* service name */,\n \t\t\t\"--stateless-rpc\", \"--advertise-refs\",\n \t\t\t\".\", NULL};\n-\t\tstruct rpc_service *svc = select_service(service_name);\n+\t\tstruct rpc_service *svc = select_service(hdr, service_name);\n \n \t\tstrbuf_addf(&buf, \"application/x-git-%s-advertisement\",\n \t\t\tsvc->name);\n-\t\thdr_str(content_type, buf.buf);\n-\t\tend_headers();\n+\t\thdr_str(hdr, content_type, buf.buf);\n+\t\tend_headers(hdr);\n \n \t\tpacket_write(1, \"# service=git-%s\\n\", svc->name);\n \t\tpacket_flush(1);\n@@ -467,9 +471,9 @@ static void get_info_refs(char *arg)\n \t\trun_service(argv, 0);\n \n \t} else {\n-\t\tselect_getanyfile();\n+\t\tselect_getanyfile(hdr);\n \t\tfor_each_namespaced_ref(show_text_ref, &buf);\n-\t\tsend_strbuf(\"text/plain\", &buf);\n+\t\tsend_strbuf(hdr, \"text/plain\", &buf);\n \t}\n \tstrbuf_release(&buf);\n }\n@@ -494,24 +498,24 @@ static int show_head_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-static void get_head(char *arg)\n+static void get_head(struct strbuf *hdr, char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tselect_getanyfile();\n+\tselect_getanyfile(hdr);\n \thead_ref_namespaced(show_head_ref, &buf);\n-\tsend_strbuf(\"text/plain\", &buf);\n+\tsend_strbuf(hdr, \"text/plain\", &buf);\n \tstrbuf_release(&buf);\n }\n \n-static void get_info_packs(char *arg)\n+static void get_info_packs(struct strbuf *hdr, char *arg)\n {\n \tsize_t objdirlen = strlen(get_object_directory());\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct packed_git *p;\n \tsize_t cnt = 0;\n \n-\tselect_getanyfile();\n+\tselect_getanyfile(hdr);\n \tprepare_packed_git();\n \tfor (p = packed_git; p; p = p->next) {\n \t\tif (p->pack_local)\n@@ -525,12 +529,12 @@ static void get_info_packs(char *arg)\n \t}\n \tstrbuf_addch(&buf, '\\n');\n \n-\thdr_nocache();\n-\tsend_strbuf(\"text/plain; charset=utf-8\", &buf);\n+\thdr_nocache(hdr);\n+\tsend_strbuf(hdr, \"text/plain; charset=utf-8\", &buf);\n \tstrbuf_release(&buf);\n }\n \n-static void check_content_type(const char *accepted_type)\n+static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n {\n \tconst char *actual_type = getenv(\"CONTENT_TYPE\");\n \n@@ -538,9 +542,9 @@ static void check_content_type(const char *accepted_type)\n \t\tactual_type = \"\";\n \n \tif (strcmp(actual_type, accepted_type)) {\n-\t\thttp_status(415, \"Unsupported Media Type\");\n-\t\thdr_nocache();\n-\t\tend_headers();\n+\t\thttp_status(hdr, 415, \"Unsupported Media Type\");\n+\t\thdr_nocache(hdr);\n+\t\tend_headers(hdr);\n \t\tformat_write(1,\n \t\t\t\"Expected POST with Content-Type '%s',\"\n \t\t\t\" but received '%s' instead.\\n\",\n@@ -549,23 +553,23 @@ static void check_content_type(const char *accepted_type)\n \t}\n }\n \n-static void service_rpc(char *service_name)\n+static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n \tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n-\tstruct rpc_service *svc = select_service(service_name);\n+\tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n-\tcheck_content_type(buf.buf);\n+\tcheck_content_type(hdr, buf.buf);\n \n-\thdr_nocache();\n+\thdr_nocache(hdr);\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-result\", svc->name);\n-\thdr_str(content_type, buf.buf);\n+\thdr_str(hdr, content_type, buf.buf);\n \n-\tend_headers();\n+\tend_headers(hdr);\n \n \targv[0] = svc->name;\n \trun_service(argv, svc->buffer_input);\n@@ -576,11 +580,13 @@ static int dead;\n static NORETURN void die_webcgi(const char *err, va_list params)\n {\n \tif (dead <= 1) {\n+\t\tstruct strbuf hdr = STRBUF_INIT;\n+\n \t\tvreportf(\"fatal: \", err, params);\n \n-\t\thttp_status(500, \"Internal Server Error\");\n-\t\thdr_nocache();\n-\t\tend_headers();\n+\t\thttp_status(&hdr, 500, \"Internal Server Error\");\n+\t\thdr_nocache(&hdr);\n+\t\tend_headers(&hdr);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\n@@ -617,7 +623,7 @@ static char* getdir(void)\n static struct service_cmd {\n \tconst char *method;\n \tconst char *pattern;\n-\tvoid (*imp)(char *);\n+\tvoid (*imp)(struct strbuf *, char *);\n } services[] = {\n \t{\"GET\", \"/HEAD$\", get_head},\n \t{\"GET\", \"/info/refs$\", get_info_refs},\n@@ -632,6 +638,21 @@ static struct service_cmd {\n \t{\"POST\", \"/git-receive-pack$\", service_rpc}\n };\n \n+static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n+{\n+\tconst char *proto = getenv(\"SERVER_PROTOCOL\");\n+\n+\tif (proto && !strcmp(proto, \"HTTP/1.1\")) {\n+\t\thttp_status(hdr, 405, \"Method Not Allowed\");\n+\t\thdr_str(hdr, \"Allow\",\n+\t\t\t!strcmp(c->method, \"GET\") ? \"GET, HEAD\" : c->method);\n+\t} else\n+\t\thttp_status(hdr, 400, \"Bad Request\");\n+\thdr_nocache(hdr);\n+\tend_headers(hdr);\n+\treturn 0;\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tchar *method = getenv(\"REQUEST_METHOD\");\n@@ -639,6 +660,7 @@ int cmd_main(int argc, const char **argv)\n \tstruct service_cmd *cmd = NULL;\n \tchar *cmd_arg = NULL;\n \tint i;\n+\tstruct strbuf hdr = STRBUF_INIT;\n \n \tset_die_routine(die_webcgi);\n \tset_die_is_recursing_routine(die_webcgi_recursing);\n@@ -659,18 +681,8 @@ int cmd_main(int argc, const char **argv)\n \t\tif (!regexec(&re, dir, 1, out, 0)) {\n \t\t\tsize_t n;\n \n-\t\t\tif (strcmp(method, c->method)) {\n-\t\t\t\tconst char *proto = getenv(\"SERVER_PROTOCOL\");\n-\t\t\t\tif (proto && !strcmp(proto, \"HTTP/1.1\")) {\n-\t\t\t\t\thttp_status(405, \"Method Not Allowed\");\n-\t\t\t\t\thdr_str(\"Allow\", !strcmp(c->method, \"GET\") ?\n-\t\t\t\t\t\t\"GET, HEAD\" : c->method);\n-\t\t\t\t} else\n-\t\t\t\t\thttp_status(400, \"Bad Request\");\n-\t\t\t\thdr_nocache();\n-\t\t\t\tend_headers();\n-\t\t\t\treturn 0;\n-\t\t\t}\n+\t\t\tif (strcmp(method, c->method))\n+\t\t\t\treturn bad_request(&hdr, c);\n \n \t\t\tcmd = c;\n \t\t\tn = out[0].rm_eo - out[0].rm_so;\n@@ -682,19 +694,19 @@ int cmd_main(int argc, const char **argv)\n \t}\n \n \tif (!cmd)\n-\t\tnot_found(\"Request not supported: '%s'\", dir);\n+\t\tnot_found(&hdr, \"Request not supported: '%s'\", dir);\n \n \tsetup_path();\n \tif (!enter_repo(dir, 0))\n-\t\tnot_found(\"Not a git repository: '%s'\", dir);\n+\t\tnot_found(&hdr, \"Not a git repository: '%s'\", dir);\n \tif (!getenv(\"GIT_HTTP_EXPORT_ALL\") &&\n \t    access(\"git-daemon-export-ok\", F_OK) )\n-\t\tnot_found(\"Repository not exported: '%s'\", dir);\n+\t\tnot_found(&hdr, \"Repository not exported: '%s'\", dir);\n \n \thttp_config();\n \tmax_request_buffer = git_env_ulong(\"GIT_HTTP_MAX_REQUEST_BUFFER\",\n \t\t\t\t\t   max_request_buffer);\n \n-\tcmd->imp(cmd_arg);\n+\tcmd->imp(&hdr, cmd_arg);\n \treturn 0;\n }\n-- \nEW\n"},{"id":"293619","messageId":"20160810123201.ylfsnzmubpmtyoaa@sigill.intra.peff.net","threadId":"43041","inReplyTo":"20160809234731.GA10310@dcvr","subject":"Re: [PATCH] http-backend: buffer headers before sending","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-10T12:32:02Z","receivedAt":"2016-08-10T19:40:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2016 at 11:47:31PM +0000, Eric Wong wrote:\n\n> Avoid waking up the readers for unnecessary context switches for\n> each line of header data being written, as all the headers are\n> written in short succession.\n> \n> It is unlikely any HTTP/1.x server would want to read a CGI\n> response one-line-at-a-time and trickle each to the client.\n> Instead, I'd expect HTTP servers want to minimize syscall and\n> TCP/IP framing overhead by trying to send all of its response\n> headers in a single syscall or even combining the headers and\n> first chunk of the body with MSG_MORE or writev.\n> \n> Verified by strace-ing response parsing on the CGI side.\n\nI don't think this is wrong to do, but it does feel like it makes the\ncode slightly more brittle (you have to pass around the strbuf and\nremember to initialize it and end_headers() when you're done), for not\nmuch benefit.\n\nUsing some kind of buffered I/O would be nicer, as then you would get\nnice-sized chunks without having to impact the code. I wonder if just\nusing stdio here would be that bad. The place it usually sucks is in\ncomplex error handling, but we don't care about that at all here (I\nthink we are basically happy to write until we get SIGPIPE).\n\nI dunno. I suspect the performance improvement from your patch is\nmarginal, but it's not like the resulting code is all _that_ complex. So\nI guess I am OK either way, just not enthused.\n\n> ---\n>   I admit I only noticed this because I was being lazy when\n>   implementing the reader-side on an HTTP server by making\n>   a single read(2) call :x\n\nThe trouble is that your HTTP server is still broken. Now it's just\nbroken in an unpredictable and racy way, because the OS may still split\nthe write at PIPE_BUF boundaries. (Though given that this is not in the\ncommit message, I suspect you know this patch is not an excuse not to\nfix your HTTP server).\n\n-Peff\n"},{"id":"293640","messageId":"20160810203611.GA24099@starla","threadId":"43041","inReplyTo":"20160810123201.ylfsnzmubpmtyoaa@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: buffer headers before sending","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-10T20:36:11Z","receivedAt":"2016-08-10T20:36:16Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Tue, Aug 09, 2016 at 11:47:31PM +0000, Eric Wong wrote:\n> \n> > Avoid waking up the readers for unnecessary context switches for\n> > each line of header data being written, as all the headers are\n> > written in short succession.\n> > \n> > It is unlikely any HTTP/1.x server would want to read a CGI\n> > response one-line-at-a-time and trickle each to the client.\n> > Instead, I'd expect HTTP servers want to minimize syscall and\n> > TCP/IP framing overhead by trying to send all of its response\n> > headers in a single syscall or even combining the headers and\n> > first chunk of the body with MSG_MORE or writev.\n> > \n> > Verified by strace-ing response parsing on the CGI side.\n> \n> I don't think this is wrong to do, but it does feel like it makes the\n> code slightly more brittle (you have to pass around the strbuf and\n> remember to initialize it and end_headers() when you're done), for not\n> much benefit.\n\nYes, I was nervous about some of these changes and had to look\nit over several times.  I think the problem was the code\ninitially assumed global state while this change localizes it;\nso this ought to make future changes easier.\n\nThanks to you and Junio for the extra eyes.\n\n> Using some kind of buffered I/O would be nicer, as then you would get\n> nice-sized chunks without having to impact the code. I wonder if just\n> using stdio here would be that bad. The place it usually sucks is in\n> complex error handling, but we don't care about that at all here (I\n> think we are basically happy to write until we get SIGPIPE).\n\nstdio to a non-blocking pipe (if so setup by a caller) could be\nproblematic portability-wise.  And reliance on SIGPIPE would\nhurt reusability if this were eventually factored into a\nstandalone daemon using libmicrohttpd or similar.\n\n> I dunno. I suspect the performance improvement from your patch is\n> marginal, but it's not like the resulting code is all _that_ complex. So\n> I guess I am OK either way, just not enthused.\n\nYes, marginal, but I still get annoyed to see extra lines from\nstrace (maybe I trace syscalls too much :x).  But I think it's\nalso a baby step which opens up potential for the future.  I\nhave nothing planned at the moment, but who knows in a year or\nfive...\n\n> > ---\n> >   I admit I only noticed this because I was being lazy when\n> >   implementing the reader-side on an HTTP server by making\n> >   a single read(2) call :x\n> \n> The trouble is that your HTTP server is still broken. Now it's just\n> broken in an unpredictable and racy way, because the OS may still split\n> the write at PIPE_BUF boundaries. (Though given that this is not in the\n> commit message, I suspect you know this patch is not an excuse not to\n> fix your HTTP server).\n\nOf course :)\n"},{"id":"293651","messageId":"xmqqlh047fod.fsf@gitster.mtv.corp.google.com","threadId":"43041","inReplyTo":"20160809234731.GA10310@dcvr","subject":"Re: [PATCH] http-backend: buffer headers before sending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-10T16:27:14Z","receivedAt":"2016-08-10T21:20:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> diff --git a/http-backend.c b/http-backend.c\n> index 0d59499..adc8c8c 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -75,55 +75,57 @@ static void format_write(int fd, const char *fmt, ...)\n>  \twrite_or_die(fd, buffer, n);\n>  }\n>  \n> -static void http_status(unsigned code, const char *msg)\n> +static void http_status(struct strbuf *hdr, unsigned code, const char *msg)\n>  {\n> -\tformat_write(1, \"Status: %u %s\\r\\n\", code, msg);\n> +\tstrbuf_addf(hdr, \"Status: %u %s\\r\\n\", code, msg);\n>  }\n\nThe idea is to pass \"struct strbuf *hdr\" around the callchain to\nhelpers so that they can all append into it instead of doing\nformat_write(), which is sensible.  This pattern continues quite a\nbit (omitted).\n\n> -static void end_headers(void)\n> +static void end_headers(struct strbuf *hdr)\n>  {\n> -\twrite_or_die(1, \"\\r\\n\", 2);\n> +\tstrbuf_add(hdr, \"\\r\\n\", 2);\n> +\twrite_or_die(1, hdr->buf, hdr->len);\n> +\tstrbuf_release(hdr);\n>  }\n\nThen end_headers() is taught to drain the buffered headers.  The\nhelpers used by other helpers in the next level of abstraction that\nemit the header and the body also need to take the buffer as their\nparameter, like this one:\n\n> -static void send_strbuf(const char *type, struct strbuf *buf)\n> +static void send_strbuf(struct strbuf *hdr,\n> +\t\t\tconst char *type, struct strbuf *buf)\n>  {\n> -\thdr_int(content_length, buf->len);\n> -\thdr_str(content_type, type);\n> -\tend_headers();\n> +\thdr_int(hdr, content_length, buf->len);\n> +\thdr_str(hdr, content_type, type);\n> +\tend_headers(hdr);\n>  \twrite_or_die(1, buf->buf, buf->len);\n>  }\n\nbecause their caller already has something to say in the header\npart.\n\n> +static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n> +{\n> +\tconst char *proto = getenv(\"SERVER_PROTOCOL\");\n> +\n> +\tif (proto && !strcmp(proto, \"HTTP/1.1\")) {\n> +\t\thttp_status(hdr, 405, \"Method Not Allowed\");\n> +\t\thdr_str(hdr, \"Allow\",\n> +\t\t\t!strcmp(c->method, \"GET\") ? \"GET, HEAD\" : c->method);\n> +\t} else\n> +\t\thttp_status(hdr, 400, \"Bad Request\");\n> +\thdr_nocache(hdr);\n> +\tend_headers(hdr);\n> +\treturn 0;\n> +}\n> +\n\nThis is like other helpers that respond with 4xx, e.g. forbidden(),\nbut it did not exist only because there was a single callsite that\nneeded this feature, which made it open-coded directly in main().\nIt was somewhat surprising to see a new helper, because the only\nthing this patch changes logically is where the output is sent, but\nthis is a very sensible refactoring.\n\n>  int cmd_main(int argc, const char **argv)\n>  {\n>  \tchar *method = getenv(\"REQUEST_METHOD\");\n> ...\n> @@ -659,18 +681,8 @@ int cmd_main(int argc, const char **argv)\n>  \t\tif (!regexec(&re, dir, 1, out, 0)) {\n>  \t\t\tsize_t n;\n>  \n> -\t\t\tif (strcmp(method, c->method)) {\n> -\t\t\t\tconst char *proto = getenv(\"SERVER_PROTOCOL\");\n> -\t\t\t\tif (proto && !strcmp(proto, \"HTTP/1.1\")) {\n> -\t\t\t\t\thttp_status(405, \"Method Not Allowed\");\n> -\t\t\t\t\thdr_str(\"Allow\", !strcmp(c->method, \"GET\") ?\n> -\t\t\t\t\t\t\"GET, HEAD\" : c->method);\n> -\t\t\t\t} else\n> -\t\t\t\t\thttp_status(400, \"Bad Request\");\n> -\t\t\t\thdr_nocache();\n> -\t\t\t\tend_headers();\n> -\t\t\t\treturn 0;\n> -\t\t\t}\n> +\t\t\tif (strcmp(method, c->method))\n> +\t\t\t\treturn bad_request(&hdr, c);\n\n... and this is where it came from.  It could have been a separate\n\"preparatory cleanup\" step, but it is so trivial that \"while at it\"\nclean-up is perfectly fine.\n\nThanks, will queue.\n\n"}]}