"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
remove gotwebd repository cache
To:
gameoftrees@openbsd.org
Date:
Fri, 17 Nov 2023 00:46:20 +0100

Download raw body.

Thread
I suspect the sharing of sock->pack_fds across cached open repositories
during requests is the source of the "unexpecte EOF" errors that
sometimes occur on my gotwebd instance.

This attempts to remove the cache again. The hard part is ensuring that
the repository is closed once the request is done. Did I get this right?

-----------------------------------------------
 remove the gotwebd repository cache
 
 It only had 4 slots so was never quite useful, and sharing of sock->pack_fds
 across cached repositories seems problematic.
 
diff 8def94257de0c243b087788186b73585ffc24f25 9f7fdfc609eabdad63f75cdf08e64f024aba8741
commit - 8def94257de0c243b087788186b73585ffc24f25
commit + 9f7fdfc609eabdad63f75cdf08e64f024aba8741
blob - b35fa0bcfb79ea056b7d4831d3597333b7341e2a
blob + b1108c45d826da11c2d77f4d5c684f9c629ea7c8
--- gotwebd/config.c
+++ gotwebd/config.c
@@ -85,11 +85,6 @@ config_getserver(struct gotwebd *env, struct imsg *ims
 	IMSG_SIZE_CHECK(imsg, srv);
 
 	memcpy(srv, p, sizeof(*srv));
-	srv->cached_repos = calloc(GOTWEBD_REPO_CACHESIZE,
-	    sizeof(*srv->cached_repos));
-	if (srv->cached_repos == NULL)
-		fatal("%s: calloc", __func__);
-	srv->ncached_repos = 0;
 
 	/* log server info */
 	log_debug("%s: server=%s fcgi_socket=%s unix_socket=%s", __func__,
blob - 807b59726fb1bd60b655c7536a08f348058f1da3
blob + 1d83512f2b41883a253253c4132db9b48cc8bae4
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -215,7 +215,7 @@ gotweb_process_request(struct request *c)
 			goto err;
 	}
 
-	switch(qs->action) {
+	switch (qs->action) {
 	case BLAME:
 		error = got_get_repo_commits(c, 1);
 		if (error) {
@@ -223,9 +223,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_blame);
-		return;
+		goto close_repo;
 	case BLOB:
 		if (binary) {
 			struct gotweb_url url = {
@@ -239,13 +239,13 @@ gotweb_process_request(struct request *c)
 			};
 
 			gotweb_reply(c, 302, NULL, &url);
-			return;
+			goto close_repo;
 		}
 
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_blob);
-		return;
+		goto close_repo;
 	case BLOBRAW:
 		if (binary)
 			r = gotweb_reply_file(c, "application/octet-stream",
@@ -267,15 +267,15 @@ gotweb_process_request(struct request *c)
 			if (fcgi_write(c, buf, len) == -1)
 				break;
 		}
-		return;
+		goto close_repo;
 	case BRIEFS:
 		error = got_get_repo_commits(c, srv->max_commits_display);
 		if (error)
 			goto err;
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_briefs);
-		return;
+		goto close_repo;
 	case COMMITS:
 		error = got_get_repo_commits(c, srv->max_commits_display);
 		if (error) {
@@ -283,9 +283,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_commits);
-		return;
+		goto close_repo;
 	case DIFF:
 		error = got_get_repo_commits(c, 1);
 		if (error) {
@@ -298,9 +298,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_diff);
-		return;
+		goto close_repo;
 	case INDEX:
 		c->t->nrepos = scandir(srv->repos_path, &c->t->repos, NULL,
 		    alphasort);
@@ -311,16 +311,16 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_index);
-		return;
+		goto close_repo;
 	case RSS:
 		error = got_get_repo_tags(c, D_MAXSLCOMMDISP);
 		if (error)
 			goto err;
 		if (gotweb_reply_file(c, rss_ctype, repo_dir->name, ".rss")
 		    == -1)
-			return;
+			goto close_repo;
 		gotweb_render_rss(c->tp);
 		return;
 	case SUMMARY:
@@ -343,7 +343,7 @@ gotweb_process_request(struct request *c)
 		}
 		qs->action = SUMMARY;
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_summary);
 		return;
 	case TAG:
@@ -358,7 +358,7 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tag);
 		return;
 	case TAGS:
@@ -368,7 +368,7 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tags);
 		return;
 	case TREE:
@@ -378,9 +378,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tree);
-		return;
+		goto close_repo;
 	case ERR:
 	default:
 		error = got_error(GOT_ERR_BAD_QUERYSTRING);
@@ -389,8 +389,13 @@ gotweb_process_request(struct request *c)
 err:
 	c->t->error = error;
 	if (gotweb_reply(c, 400, "text/html", NULL) == -1)
-		return;
+		goto close_repo;
 	gotweb_render_page(c->tp, gotweb_render_error);
+close_repo:
+	if (c->t->repo) {
+		got_repo_close(c->t->repo);
+		c->t->repo = NULL;
+	}
 }
 
 struct server *
@@ -860,6 +865,10 @@ gotweb_render_index(struct template *tp)
 				    sd_dent[d_i]->d_name, error->msg);
 			gotweb_free_repo_dir(repo_dir);
 			repo_dir = NULL;
+			if (t->repo) {
+				got_repo_close(t->repo);
+				t->repo = NULL;
+			}
 			d_skipped++;
 			continue;
 		}
@@ -1104,75 +1113,13 @@ gotweb_render_absolute_url(struct request *c, struct g
 	return gotweb_render_url(c, url);
 }
 
-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 >= GOTWEBD_REPO_CACHESIZE) {
-		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;
-}
-
-static const struct got_error *
 gotweb_load_got_path(struct request *c, struct repo_dir *repo_dir)
 {
 	const struct got_error *error = NULL;
 	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;
 
@@ -1209,13 +1156,9 @@ done:
 		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 = got_repo_open(&t->repo, repo_dir->path, NULL, sock->pack_fds);
+	if (error)
+		goto done;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
 	    repo_dir->path, dirfd(dt));
 	if (error)
blob - a76ef44049655b82575dc5a398b4739eeb44efbc
blob + 71b34a0258f2a201a28b0745032caf69c7fcd5e6
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -47,7 +47,6 @@
 #define GOTWEBD_MAXNAME		 64
 #define GOTWEBD_MAXPORT		 6
 #define GOTWEBD_NUMPROC		 3
-#define GOTWEBD_REPO_CACHESIZE	 4
 #define GOTWEBD_SOCK_FILENO	 3
 
 #define PROC_MAX_INSTANCES	 32
@@ -290,18 +289,10 @@ 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;
-	int		 ncached_repos;
-
 	char		 name[GOTWEBD_MAXTEXT];
 
 	char		 repos_path[PATH_MAX];
blob - a16821af966e854627051ef7d1ddb100a024c3d1
blob + 4ac43e3ddee0f601ea33b3a80f934242683cdc4c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -399,7 +399,6 @@ sockets_shutdown(void)
 {
 	struct server *srv, *tsrv;
 	struct socket *sock, *tsock;
-	int i;
 
 	sockets_purge(gotwebd_env);
 
@@ -411,11 +410,8 @@ sockets_shutdown(void)
 	}
 
 	/* clean servers */
-	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);
+	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv)
 		free(srv);
-	}
 
 	free(gotwebd_env);
 }