From: Omar Polo <op@omarpolo.com>
Subject: Re: remove gotwebd repository cache
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 17 Nov 2023 10:16:32 +0100

On 2023/11/17 10:06:46 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Nov 17, 2023 at 09:09:48AM +0100, Stefan Sperling wrote:
> > On Fri, Nov 17, 2023 at 12:46:20AM +0100, Stefan Sperling wrote:
> > > 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?
> > 
> > There previous diff still file descriptor leaks, causing fd exhaustion
> > resulting in errors like "bad exit code from unprivileged process".
> > Presumbaly because a forked child ran out of available fd slots early on.
> > One sockets worker had 1024 open files, which is the hard limit.
> > 
> > New diff, with all the early returns in process_request changed to
> > close_repo gotos. Just in case I am also letting gotweb_load_got_path()
> > close open repositories on error.
> 
> On IRC, Omar pointed out a better place to close the per-request repository.

ok op@, two nits below

> [...]
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> [...]
> @@ -860,6 +862,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;
> +			}

since you now close the repo in gotweb_load_got_path() on failure
(thanks!) this hunk is not needed.

>  			d_skipped++;
>  			continue;
>  		}
> @@ -869,6 +875,11 @@ gotweb_render_index(struct template *tp)
>  
>  		r = gotweb_render_repo_fragment(c->tp, repo_dir);
>  		gotweb_free_repo_dir(repo_dir);
> +		repo_dir = NULL;
> +		if (t->repo) {
> +			got_repo_close(t->repo);
> +			t->repo = NULL;
> +		}

likewise here t->repo can't be NULL

>  		if (r == -1)
>  			return -1;
>