Diff options
| author | Bryce Kwon <bryce@brycekwon.com> | |
|---|---|---|
| committer | Bryce Kwon <bryce@brycekwon.com> | |
| commit | ||
| parent | ||
| tree | ||
| download | ||
End a request quietly when the client has gone
A write that failed with EPIPE on an uncached page died into an error
page, which died again at exit when the page could not be sent either,
and git logged a recursion warning on every dropped connection. A
filter that exits early fails a write the same way, so the quiet exit
is kept for writes that reach the client itself.
| -rw-r--r-- | source/cache.c | 3 | |
| -rw-r--r-- | source/cgit.c | 4 | |
| -rw-r--r-- | source/filter.c | 5 | |
| -rw-r--r-- | source/filter.h | 6 | |
| -rw-r--r-- | source/html.c | 20 | |
| -rwxr-xr-x | tests/t0303-robustness.sh | 16 |
6 files changed, 50 insertions, 4 deletions
diff --git a/source/cache.c b/source/cache.c index 8c81f5c..62bb623 100644 --- a/source/cache.c +++ b/source/cache.c @@ -175,7 +175,8 @@ static int serve_slot(struct cache_slot *slot) int err; err = print_slot(slot); - if (err) + // A client that went away mid-page is not trouble worth a line. + if (err && err != EPIPE) log_error("[cgit] Unable to send slot %s: %s (%d)\n", slot->path, strerror(err), err); return err; } diff --git a/source/cgit.c b/source/cgit.c index 3e47dbe..633b011 100644 --- a/source/cgit.c +++ b/source/cgit.c @@ -1380,7 +1380,9 @@ int cmd_main(int argc, const char **argv) strbuf_release(&cache_key); cgit_cleanup_filters(); - if (err) + // A slot that could not be sent because the client went away is not + // worth an error page nobody will read. + if (err && err != EPIPE) cgit_print_error("Error processing page: %s (%d)", strerror(err), err); return err; } diff --git a/source/filter.c b/source/filter.c index dd1eaf0..6e16537 100644 --- a/source/filter.c +++ b/source/filter.c @@ -450,6 +450,11 @@ void cgit_abort_filters(void) #endif } +int cgit_filter_holds_stdout(void) +{ + return running_exec != NULL; +} + static const struct { const char *prefix; struct cgit_filter *(*create)(const char *cmd, int argument_count); diff --git a/source/filter.h b/source/filter.h index b5a478f..70fa4ff 100644 --- a/source/filter.h +++ b/source/filter.h @@ -59,4 +59,10 @@ extern void cgit_cleanup_filters(void); */ extern void cgit_abort_filters(void); +/* + * Whether an exec filter holds stdout at the moment, which is when a failed + * write means the filter went away rather than the client. + */ +extern int cgit_filter_holds_stdout(void); + #endif // CGIT_FILTER_H diff --git a/source/html.c b/source/html.c index 96b1b52..caa5a29 100644 --- a/source/html.c +++ b/source/html.c @@ -10,6 +10,7 @@ #include "cgit.h" #include "cache.h" +#include "filter.h" #include "html.h" #define HTML_WRITE_BUFSIZE (64 * 1024) @@ -57,6 +58,8 @@ static struct strbuf *capture; static void write_out(const char *data, size_t size) { + int err; + // A blob or snapshot is well past what one write can move onto a // pipe, so short writes are resumed rather than reported. while (size) { @@ -69,12 +72,25 @@ static void write_out(const char *data, size_t size) } if (!size) return; + err = errno; // While a cache slot is being filled stdout is a file, and a full disk // must not cost the visitor the page. Abandoning the fill puts stdout // back on the client along with what the file already holds, so only // the rest is written again. - if (!cache_abandon_fill() || write_in_full(STDOUT_FILENO, data, size) < 0) - die_errno("Unable to write the page"); + if (cache_abandon_fill()) { + if (write_in_full(STDOUT_FILENO, data, size) < 0) + die_errno("Unable to write the page"); + return; + } + // A client that has gone away fails the write with EPIPE and leaves + // nobody to read an error page, so the request ends quietly. A filter + // that exited early fails it the same way, and that page has a reader. + if (err == EPIPE && !cgit_filter_holds_stdout()) { + html_discard(); + exit(0); + } + errno = err; + die_errno("Unable to write the page"); } /* diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.sh index 55a83c3..fd1c02e 100755 --- a/tests/t0303-robustness.sh +++ b/tests/t0303-robustness.sh @@ -529,6 +529,22 @@ test_expect_success 'the dumb transport withholds the alternates file' ' grep "^Status: 200" tmp ' +# A client that goes away mid-page leaves nobody to send an error page to, +# so the request ends quietly instead of dying twice into the log. The page +# has to outgrow the pipe for the write after the reader has gone to fail. +test_expect_success 'a client that disconnects ends the request quietly' ' + ( + cd repos/rob && + awk "BEGIN{for(i=0;i<20000;i++) print \"line \" i}" >long.txt && + git add long.txt && + git commit -m long + ) && + { robq "url=rob/tree/long.txt" 2>err; echo $? >status; } | head -c 1 >/dev/null && + test "$(cat status)" = 0 && + ! grep -i "broken pipe" err && + ! grep "die()" err +' + # The about page redirects to its trailing-slash form so relative links # resolve, and the branch asked for has to survive that hop, as does the # hop back to the summary of a repository without a readme. |