On 2022/08/31 11:40:44 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> 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?

reads fine; just one note below but it's ok for me

> 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);

We `goto err' also when fcgi_printf returns -1, i.e. upon premature
client disconnection.  (OK, also if asprintf fails, but that error is
not propagate up to here and it's not relevant for the cache anyway.)
I think it should look if error is non-NULL.

There is another issue that I think is worth being raised: clients can
force errors by playing with the query string.  By passing invalid
query parameters they can force all kinds of erros being returned here
(from no git directory, to no object, to no file entry...) and so
making us invalidate the cache.

The cache is just intended to be an optimization so this is probably
fine.

> +	}
>  	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);
>  }