On 2022/09/01 17:44:17 +0200, Omar Polo <op@omarpolo.com> wrote:
> On 2022/08/25 17:41:46 +0200, Omar Polo <op@omarpolo.com> wrote:
> > I'm still seeing gotwebd increasing its memory usage over time so I'm
> > still searching for leaks.
> > 
> > [...]
> > 
> > this still doesn't explain why gotwebd reached 600M of RES in hours,
> > so there's probably more.
> 
> one big leak was in gotweb_render_index where we fail to free the data
> allocated by scandir(3)
> 
> with this is, gotwebd memory consumption didn't raised after a few
> thousands requests (I'm running ftp in a loop against it)! \o/

I phrased this poorly; i'm going page by page and the listing page is
now memleaks free (well, for the most part at least :D)

diff below tries to fix most of the leaks in the repo summary page.
definitely needs more eyeballs because i'm not sure anymore what I'm
doing.

There are a simple fix in it, like forgetting to free some strings or
close the commits in some code paths, but the big leak is
`repo_commit' that get filled each time with various strings, and each
time we leak the data of the previous run.  To fix it I'm trying to
avoid doing a double got_get_repo_commit in the loop and doing it just
once.  From a quick look seems to even work! :O

diff /home/op/w/got
commit - 2db401bd3a14512e3a1f1cbe686fff37b2c56764
path + /home/op/w/got
blob - ac3b54fcdf929d17f3aea822d4162937114cf8a7
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -332,12 +332,12 @@ got_get_repo_commits(struct request *c, int limit)
 	struct got_commit_object *commit = NULL;
 	struct got_reflist_head refs;
 	struct got_reference *ref;
-	struct repo_commit *repo_commit = NULL;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
 	struct got_repository *repo = t->repo;
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
+	char *commit_id = NULL;
 	char *in_repo_path = NULL, *repo_path = NULL, *file_path = NULL;
 	int chk_next = 0, chk_multi = 0, commit_found = 0;
 	int obj_type, limit_chk = 0;
@@ -355,10 +355,6 @@ got_get_repo_commits(struct request *c, int limit)
 		goto done;
 	}
 
-	error = got_init_repo_commit(&repo_commit);
-	if (error)
-		goto done;
-
 	/*
 	 * XXX: jumping directly to a commit id via
 	 * got_repo_match_object_id_prefix significantly improves performance,
@@ -440,9 +436,11 @@ got_get_repo_commits(struct request *c, int limit)
 		goto done;
 
 	for (;;) {
+		struct repo_commit *repo_commit = NULL;
+
 		if (limit_chk == ((limit * qs->page) - (limit - 1)) &&
-		    commit_found == 0 && repo_commit->commit_id != NULL) {
-			t->prev_id = strdup(repo_commit->commit_id);
+		    commit_found == 0 && commit_id != NULL) {
+			t->prev_id = strdup(commit_id);
 			if (t->prev_id == NULL) {
 				error = got_error_from_errno("strdup");
 				goto done;
@@ -468,40 +466,43 @@ got_get_repo_commits(struct request *c, int limit)
 		if (error)
 			goto done;
 
+		error = got_init_repo_commit(&repo_commit);
+		if (error)
+			goto done;
+
 		error = got_get_repo_commit(c, repo_commit, commit,
-		    &refs, id);
+		       &refs, id);
 		if (error)
 			goto done;
+		free(commit_id);
+		commit_id = strdup(repo_commit->commit_id);
+		if (commit_id == NULL) {
+			error = got_error_from_errno("strdup");
+			gotweb_free_repo_commit(repo_commit);
+			goto done;
+		}
 
+		free(id);
+		id = NULL;
+
 		if (qs->commit != NULL && commit_found == 0 && limit != 1) {
-			if (strcmp(qs->commit, repo_commit->commit_id) == 0)
+			if (strcmp(qs->commit, commit_id) == 0)
 				commit_found = 1;
 			else if (qs->file != NULL && strlen(qs->file) > 0 &&
 			    qs->page == 0)
 				commit_found = 1;
 			else {
 				limit_chk++;
-				free(id);
-				id = NULL;
+				got_object_commit_close(commit);
+				commit = NULL;
+				got_ref_list_free(&refs);
+				gotweb_free_repo_commit(repo_commit);
 				continue;
 			}
 		}
 
-		struct repo_commit *new_repo_commit = NULL;
-		error = got_init_repo_commit(&new_repo_commit);
-		if (error)
-			goto done;
+		TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry);
 
-		TAILQ_INSERT_TAIL(&t->repo_commits, new_repo_commit, entry);
-
-		error = got_get_repo_commit(c, new_repo_commit, commit,
-		    &refs, id);
-		if (error)
-			goto done;
-
-		free(id);
-		id = NULL;
-
 		if (limit == 1 && chk_multi == 0 &&
 		    srv->max_commits_display != 1)
 			commit_found = 1;
@@ -515,7 +516,7 @@ got_get_repo_commits(struct request *c, int limit)
 			 */
 			if (chk_next && (qs->action == BRIEFS ||
 			    qs->action == COMMITS || qs->action == SUMMARY)) {
-				t->next_id = strdup(new_repo_commit->commit_id);
+				t->next_id = strdup(commit_id);
 				if (t->next_id == NULL) {
 					error = got_error_from_errno("strdup");
 					goto done;
@@ -524,9 +525,9 @@ got_get_repo_commits(struct request *c, int limit)
 					got_object_commit_close(commit);
 					commit = NULL;
 				}
-				TAILQ_REMOVE(&t->repo_commits, new_repo_commit,
+				TAILQ_REMOVE(&t->repo_commits, repo_commit,
 				    entry);
-				gotweb_free_repo_commit(new_repo_commit);
+				gotweb_free_repo_commit(repo_commit);
 				goto done;
 			}
 		}
@@ -544,13 +545,17 @@ got_get_repo_commits(struct request *c, int limit)
 		}
 	}
 done:
-	gotweb_free_repo_commit(repo_commit);
 	if (commit)
 		got_object_commit_close(commit);
 	if (graph)
 		got_commit_graph_close(graph);
+	if (commit)
+		got_object_commit_close(commit);
 	got_ref_list_free(&refs);
+	free(commit_id);
 	free(file_path);
+	free(in_repo_path);
+	free(file_path);
 	free(repo_path);
 	free(id);
 	return error;