From 6a9b662b50927b2dbe3c8d383f04d1d7b649b28f Mon Sep 17 00:00:00 2001 From: Bryce Kwon Date: Sat, 8 Aug 2026 12:49:57 -1000 Subject: Gather page output into one buffer --- source/cache.c | 1 + source/cgit.c | 3 +++ source/filter.c | 10 +++++++++ source/html.c | 47 +++++++++++++++++++++++++++++++++++++++-- source/html.h | 4 ++++ source/ui-shared.c | 4 ++++ tests/t0020-validate-cache.sh | 49 +++++++++++++++++++++++++++++++++++++++++++ tests/t0111-filter.sh | 16 ++++++++++++++ 8 files changed, 132 insertions(+), 2 deletions(-) diff --git a/source/cache.c b/source/cache.c index 105ecfe..e916728 100644 --- a/source/cache.c +++ b/source/cache.c @@ -242,6 +242,7 @@ static int fill_slot(struct cache_slot *slot) slot->fn(); /* Make sure any buffered data is flushed to the file */ + html_flush(); if (fflush(stdout)) return errno; diff --git a/source/cgit.c b/source/cgit.c index 5cb52fd..2ecfb4f 100644 --- a/source/cgit.c +++ b/source/cgit.c @@ -1087,6 +1087,9 @@ int cmd_main(int argc, const char **argv) isolate_git_environment(); cgit_init_filters(); atexit(cgit_cleanup_filters); + // Registered second so it runs first, since exit is reached from error + // paths and from the HEAD shortcut with a page still buffered. + atexit(html_flush); set_die_routine(cgit_die_routine); prepare_context(); diff --git a/source/filter.c b/source/filter.c index e15a5e9..f195534 100644 --- a/source/filter.c +++ b/source/filter.c @@ -216,6 +216,10 @@ static inline int hook_lua_filter(lua_State *lua_state, void (*fn)(const char *t save_filter = current_write_filter; unhook_write(); fn(str); + // fn buffers, so empty it while the hook is still off. Re-hooking first + // would send the page's own bytes back into the filter that asked for + // them to be written. + html_flush(); hook_write(save_filter, save_filter_write); return 0; @@ -368,6 +372,9 @@ int cgit_open_filter(struct cgit_filter *filter, ...) va_list ap; if (!filter) return 0; + // Whatever is still buffered belongs to the page, not to the filter + // that is about to take over stdout. + html_flush(); va_start(ap, filter); result = filter->open(filter, ap); va_end(ap); @@ -378,6 +385,9 @@ int cgit_close_filter(struct cgit_filter *filter) { if (!filter) return 0; + // Anything written while the filter was open is meant for it, so this + // has to go out before the filter hands stdout back. + html_flush(); return filter->close(filter); } diff --git a/source/html.c b/source/html.c index c3ab696..dbb0a57 100644 --- a/source/html.c +++ b/source/html.c @@ -9,6 +9,9 @@ #include "cgit.h" #include "html.h" #include "url.h" +#define HTML_WRITE_BUFSIZE (64 * 1024) +static char html_buf[HTML_WRITE_BUFSIZE]; +static size_t html_buflen; /* Percent-encoding of each character, except: a-zA-Z0-9!$()*,./:;@- */ static const char* url_escape_table[256] = { @@ -78,12 +81,52 @@ char *cgit_fmtalloc(const char *format, ...) return strbuf_detach(&sb, NULL); } -void html_raw(const char *data, size_t size) +/* + * Page output is collected here and written out in whole buffers. A page is + * built from a great many small fragments, and writing each one cost a syscall + * apiece: a tree listing spent more time entering the kernel than generating + * anything. + * + * cgit does not own stdout by itself, so the buffer has to be emptied before + * anything else writes there. Those points are a filter taking over stdout, + * the header block that precedes output produced by git itself, the cache + * slot being measured, and process exit. + */ +static void html_write(const char *data, size_t size) { - if (write(STDOUT_FILENO, data, size) != size) + // A blob, snapshot or patch reaches this with a size well past what one + // write can move onto a pipe, so a short write is ordinary rather than + // an error and has to be resumed instead of reported. + if (write_in_full(STDOUT_FILENO, data, size) < 0) die_errno("write error on html output"); } +void html_flush(void) +{ + size_t len = html_buflen; + + if (!len) + return; + // Clear the length first: html_write can die, and the error page it + // produces would otherwise try to flush the same bytes again. + html_buflen = 0; + html_write(html_buf, len); +} + +void html_raw(const char *data, size_t size) +{ + if (size >= HTML_WRITE_BUFSIZE) { + // Nothing is gained by copying a blob through the buffer. + html_flush(); + html_write(data, size); + return; + } + if (html_buflen + size > HTML_WRITE_BUFSIZE) + html_flush(); + memcpy(html_buf + html_buflen, data, size); + html_buflen += size; +} + void html(const char *txt) { html_raw(txt, strlen(txt)); diff --git a/source/html.h b/source/html.h index 7572623..353254e 100644 --- a/source/html.h +++ b/source/html.h @@ -13,6 +13,10 @@ extern void html(const char *txt); * blob limits do not bound. */ #define HTML_BATCH (64 * 1024) +/* Write out whatever page output is still buffered. Call before anything + * other than html_raw writes to stdout. */ +extern void html_flush(void); + __attribute__((format (printf,1,2))) extern void htmlf(const char *format,...); diff --git a/source/ui-shared.c b/source/ui-shared.c index 125a235..23da286 100644 --- a/source/ui-shared.c +++ b/source/ui-shared.c @@ -808,6 +808,10 @@ void cgit_print_http_headers(void) if (ctx.page.etag) htmlf("ETag: \"%s\"\n", ctx.page.etag); html("\n"); + // Several pages follow the header block with output produced by git + // rather than by html_raw, the patch, raw diff and snapshot views among + // them. Emptying the buffer here keeps the headers ahead of it. + html_flush(); if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")) exit(0); } diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.sh index 657765d..7e6334c 100755 --- a/tests/t0020-validate-cache.sh +++ b/tests/t0020-validate-cache.sh @@ -75,4 +75,53 @@ test_expect_success 'verify cache-size=1021' ' test_cmp output.full output.second ' +# --- A cached page must hold everything the uncached one produced ----------- +# Page output is buffered, so the slot is only complete if the buffer is +# emptied before the generated content is measured. The blob here is larger +# than one buffer, so a missing flush would truncate the cached copy. +test_expect_success 'set up a repo with a page larger than the output buffer' ' + mkrepo repos/bigpage 1 && + ( + cd repos/bigpage && + awk "BEGIN{for(i=0;i<20000;i++) print \"line \" i \" of the big file\"}" >big.txt && + git add big.txt && + git commit -m "add big.txt" + ) && + { + echo "virtual-root=/" && + echo "cache-root=$PWD/cache2" && + echo "cache-size=64" && + echo "enable-tree-linenumbers=1" && + echo "repo.url=bigpage" && + echo "repo.path=$PWD/repos/bigpage/.git" + } >bigrc && + rm -rf cache2 && mkdir cache2 +' + +bigq() { CGIT_CONFIG="$PWD/bigrc" QUERY_STRING="$1" cgit; } + +test_expect_success 'the first request fills the slot and the second replays it' ' + bigq "url=bigpage/tree/big.txt" >big.first && + test $(wc -c big.second && + strip_headers big.first.body && + strip_headers big.second.body && + test_cmp big.first.body big.second.body +' + +test_expect_success 'the cached body matches one generated with the cache off' ' + sed -e "s/^cache-size=64$/cache-size=0/" bigrc >bignocache && + CGIT_CONFIG="$PWD/bignocache" QUERY_STRING="url=bigpage/tree/big.txt" cgit >big.nocache && + strip_headers big.nocache.body && + # The footer carries the time the page was generated, which differs + # between the two runs by construction. + sed -e "s/generated by .*//" big.nocache.body >big.a && + sed -e "s/generated by .*//" big.second.body >big.b && + test_cmp big.a big.b +' + +test_expect_success 'the page ends where it should, so nothing was dropped' ' + tail -c 200 big.second.body | grep "" +' + test_done diff --git a/tests/t0111-filter.sh b/tests/t0111-filter.sh index 2fdc366..da4172f 100755 --- a/tests/t0111-filter.sh +++ b/tests/t0111-filter.sh @@ -41,6 +41,22 @@ do test_expect_success "check whether the $prefix email filter works for committers" ' grep " commit C O MITTER <COMMITTER@EXAMPLE.COM>" tmp ' + + # Page output is buffered, so anything written before a filter opens has + # to leave the buffer before the filter takes over stdout, and anything + # written while it is open has to leave before stdout is handed back. + # A missed flush reorders the page rather than losing it, so check that + # the markup around the filtered text is still on the right side of it. + test_expect_success "the $prefix source filter output stays inside its cell" " + cgit_url 'filter-$prefix/tree/a%2bb' >tmp && + tr -d '\n' flat.out && + grep 'a+b HELLO' flat.out + " + + test_expect_success "the $prefix about filter output stays inside its div" " + cgit_url 'filter-$prefix/about/' >tmp && + grep \"
a+b HELLO\" tmp + " done test_done -- cgit v2.8.0