On Wed, Aug 31, 2022 at 11:20:45AM +0200, Stefan Sperling wrote:
> On Wed, Aug 31, 2022 at 10:54:15AM +0200, Stefan Sperling wrote:
> > gotwebd currently opens repositories per-request (and usually closes
> > them once the request is done, but fails to close them under certain
> > error conditions).
> > 
> > With this patch we keep a list of cached repositories in the server
> > context. Only one cached repo can be used when serving a request
> > because we only have one set of pack_fd temp files available.
> > 
> > However, the cache reduces per-request overhead and also ensures
> > that repositories get closed properly as they cycle out of cache
> > and when the server terminates.
> > 
> > ok?
> 
> Update version which fixes an off-by-one found by op@, and undoes
> the effects of memmove() if we fail to add a new cache element
> after eviting an existing one.

And another update to address a problem I found during testing.
If repositories are updated with new commits while gotwebd is running
then gotwebd can run into errors while accessing the repository.

For example, I saw it run into this:

gotwebd experienced an error: open: \
	/got/public/got.git/objects/69/02644eb5884f064faf1f771aaef6bf956e485d: \
	No such file or directory

This error kept repeating while I reloaded the page in the browser.

This new version of the patch invalidates the repo cache on error,
causing everything to be refreshed when the next request comes in.

ok?

diff e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951 072a608f2aa900bf9f5a2dc39c593f020580f35d
commit - e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951
commit + 072a608f2aa900bf9f5a2dc39c593f020580f35d
blob - 72a93ffe88a0fd5d23ee1850893733ac1c37c39c
blob + 095c430b0281d6659d2f08e87158bb71d41a4285
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -110,6 +110,78 @@ static void gotweb_free_repo_dir(struct repo_dir *);
 
 struct server *gotweb_get_server(uint8_t *, uint8_t *);
 
+static void
+invalidate_repo_cache(struct server *srv)
+{
+	int i;
+
+	for (i = 0; i < srv->ncached_repos; i++)
+		got_repo_close(srv->cached_repos[i].repo);
+	memset(srv->cached_repos, 0, sizeof(srv->cached_repos));
+	srv->ncached_repos = 0;
+}
+
+static struct got_repository *
+find_cached_repo(struct server *srv, const char *path)
+{
+	int i;
+
+	for (i = 0; i < srv->ncached_repos; i++) {
+		if (strcmp(srv->cached_repos[i].path, path) == 0)
+			return srv->cached_repos[i].repo;
+	}
+
+	return NULL;
+}
+
+static const struct got_error *
+cache_repo(struct got_repository **new, struct server *srv,
+    struct repo_dir *repo_dir, struct socket *sock)
+{
+	const struct got_error *error = NULL;
+	struct got_repository *repo;
+	struct cached_repo *cr;
+	int evicted = 0;
+
+	if (srv->ncached_repos >= nitems(srv->cached_repos)) {
+		cr = &srv->cached_repos[srv->ncached_repos - 1];
+		error = got_repo_close(cr->repo);
+		memset(cr, 0, sizeof(*cr));
+		srv->ncached_repos--;
+		if (error)
+			return error;
+		memmove(&srv->cached_repos[1], &srv->cached_repos[0],
+		    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		cr = &srv->cached_repos[0];
+		evicted = 1;
+	} else {
+		cr = &srv->cached_repos[srv->ncached_repos];
+	}
+
+	error = got_repo_open(&repo, repo_dir->path, NULL, sock->pack_fds);
+	if (error) {
+		if (evicted) {
+			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
+			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		}
+		return error;
+	}
+
+	if (strlcpy(cr->path, repo_dir->path, sizeof(cr->path))
+	    >= sizeof(cr->path)) {
+		if (evicted) {
+			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
+			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		}
+		return got_error(GOT_ERR_NO_SPACE);
+	}
+
+	cr->repo = repo;
+	srv->ncached_repos++;
+	*new = repo;
+	return NULL;
+}
+
 void
 gotweb_process_request(struct request *c)
 {
@@ -278,6 +350,13 @@ render:
 
 	goto done;
 err:
+	if (srv != NULL) {
+		/*
+		 * We might have run into problems due to stale repository
+		 * caches. Close all repositories to try again later.
+		 */
+		invalidate_repo_cache(srv);
+	}
 	if (html && fcgi_printf(c, "<div id='err_content'>") == -1)
 		return;
 	if (fcgi_printf(c, "%s", err) == -1)
@@ -292,8 +371,6 @@ err:
 	if (html && fcgi_printf(c, "</div>\n") == -1)
 		return;
 done:
-	if (c->t->repo != NULL && qs && qs->action != INDEX)
-		got_repo_close(c->t->repo);
 	if (html && srv != NULL)
 		gotweb_render_footer(c);
 }
@@ -1096,9 +1173,6 @@ render:
 
 		gotweb_free_repo_dir(repo_dir);
 		repo_dir = NULL;
-		error = got_repo_close(t->repo);
-		if (error)
-			goto done;
 		t->next_disp++;
 		if (d_disp == srv->max_repos_display)
 			break;
@@ -1918,6 +1992,7 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	struct socket *sock = c->sock;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
+	struct got_repository *repo = NULL;
 	DIR *dt;
 	char *dir_test;
 	int opened = 0;
@@ -1977,9 +2052,13 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	} else
 		opened = 1;
 done:
-	error = got_repo_open(&t->repo, repo_dir->path, NULL, sock->pack_fds);
-	if (error)
-		goto err;
+	repo = find_cached_repo(srv, repo_dir->path);
+	if (repo == NULL) {
+		error = cache_repo(&repo, srv, repo_dir, sock);
+		if (error)
+			goto err;
+	}
+	t->repo = repo;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
 	    repo_dir->path);
 	if (error)
blob - ae08f3df1ae9fb58a5fac19a6fcfd02a1aad9247
blob + 74ee67ae6734f6af0af3ab58f3276ceeccc8863a
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -46,6 +46,7 @@
 #define GOTWEBD_MAXPORT		 6
 #define GOTWEBD_NUMPROC		 3
 #define GOTWEBD_MAXIFACE	 16
+#define GOTWEBD_REPO_CACHESIZE	 4
 
 /* GOTWEB DEFAULTS */
 #define MAX_QUERYSTRING		 2048
@@ -250,10 +251,18 @@ struct address {
 };
 TAILQ_HEAD(addresslist, address);
 
+struct cached_repo {
+	char path[PATH_MAX];
+	struct got_repository *repo;
+};
+
 struct server {
 	TAILQ_ENTRY(server)	 entry;
 	struct addresslist	al;
 
+	struct cached_repo	cached_repos[GOTWEBD_REPO_CACHESIZE];
+	int		 ncached_repos;
+
 	char		 name[GOTWEBD_MAXTEXT];
 
 	char		 repos_path[PATH_MAX];
blob - 7e510af9c4ae6f2609874519758443d006c1afc3
blob + c0691c5575dca8c931af0e8e30d9e086b5a5bccc
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -51,6 +51,7 @@
 
 #include "got_error.h"
 #include "got_opentemp.h"
+#include "got_repository.h"
 
 #include "proc.h"
 #include "gotwebd.h"
@@ -351,6 +352,7 @@ sockets_shutdown(void)
 {
 	struct server *srv, *tsrv;
 	struct socket *sock, *tsock;
+	int i;
 
 	sockets_purge(gotwebd_env);
 
@@ -362,8 +364,11 @@ sockets_shutdown(void)
 	}
 
 	/* clean servers */
-	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv)
+	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv) {
+		for (i = 0; i < srv->ncached_repos; i++)
+			got_repo_close(srv->cached_repos[i].repo);
 		free(srv);
+	}
 
 	free(gotwebd_env);
 }