Skip to content

Potential memory leak on Sql.Db.Close() #1319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Harmos274 opened this issue Feb 20, 2025 · 6 comments
Open

Potential memory leak on Sql.Db.Close() #1319

Harmos274 opened this issue Feb 20, 2025 · 6 comments

Comments

@Harmos274
Copy link

Harmos274 commented Feb 20, 2025

sqlite3 version: github.com/mattn/go-sqlite3 v1.14.22

Hello, i would like to know if i'm doing something wrong here... It seems that sql.Db.Close() does not release allocated memory when called.

In fact, i suspect it to cause a memory leak.

I have a project that is a net/http api that exposes a GET /instances/{instance}/jobs/{jobID}/status route.

  • instance is a hash string that i get the name of an sqlite database file from.
  • jobID is the ID of a job in the given database.

My service will take the database {instance}.sqlite, opens it and query the status of the job {jobID}, Afterward it will close the given database.

flowchart LR;
    A[Http GET]-->B[net/http API];
    B-->C[Sql.Open instance.sqlite];
    C-->D[Query Job status];
    D-->E[Sql.DB.Close instance.sqlite];
    E-->F[Http response];
Loading

Here's a code sample that shows how i open and close both of my databases.

func onDatabaseContext[T any](ctx context.Context, dbManager SetupDB, instance string, onDBFunc func(ctx context.Context, writeDB, readDB *sql.DB) (T, error)) (written T, err error) {
	var writeDB, readDB *sql.DB

        // Compute sqlite db path from instance number
	path := dbManager.dbPath(instance)

	// Write in sqlite is mono threaded, therefore we use a multithreaded read db and mono threaded write db
	writeDB, err = sql.Open("sqlite3", fmt.Sprintf("file:%s?mode=rw&_journal=WAL&_timeout=5000&_txlock=immediate", path))
	if err != nil {
		return
	}

	readDB, err = sql.Open("sqlite3", fmt.Sprintf("file:%s?mode=ro&_timeout=5000&parseTime=true", path))
	if err != nil {
		return
	}

	err = initDB(ctx, writeDB)

	if err != nil {
		return
	}

	res, err := onDBFunc(ctx, writeDB, readDB)

	writeDB.Close()
	readDB.Close()

	return res, err
}

The problem is that when i close my Sql.Db handles, the allocated memory is not released, even after some time. I actually work on a lot of different sql databases which causes my service to crash from OOM error eventually.

I'm sure my pattern is not efficient, it is a proof of concept. But i would expect resources to be released when i close my handles, or at least be garbage collected after some time, which does not happen.

Could someone explain to me what i am doing wrong please ?

Thank you for your time.

@yafethtb
Copy link

I'm new to Go, but isn't it better to put writeDB.Close() and readDB.close(0 in defer for this scenario? Correct me if I'm wrong, but for each error checking, if the error is not nil, the function will return an empty value. Other codes below whichever failed error checking will not executed after that. So if one of the error checking fails, the function will just finish without even touching the writeDB.close() and readDB.close(). By using defer it make sure that whatever happens, the database will be closed before the function is close.

@Harmos274
Copy link
Author

absolutely! tbf it was the original version of my code, i wanted to be sure that .Close() is called after onDbFunc(), to exclude the hypothesis of "defer is not executed after my closure", the problem i'm talking about in that issue is happening when none of the previous calls return an error

@jmcdonagh
Copy link

Well, firstly and most obviously you are eating errors and returning early. This isn't how you should handle errors in Go. your Close calls should be done in deferrals immediately after checking whether Open returned an error or not.

As far as GC/memory leak goes, does your onDbFunc make use of goroutines perhaps? i would get rid of the callback functionality and run some tests just to confirm. Maybe something there holding onto a reference.

@yafethtb
Copy link

Issue #1328 also faces a memory leak problem. It's probably similar to what you've faced.

@Harmos274
Copy link
Author

Harmos274 commented Apr 24, 2025

@jmcdonagh as i said earlier i was facing the same issue when i was using defer i'm not sure my error would come from this part of the code (but yea in this case errors cause a memory leak, i wasn't facing any error tho, everything was fine during the execution)

no goroutines at all in the callback, just classic sql execution mainly select and return the scanned rows (i close my rows)

@Harmos274
Copy link
Author

@yafethtb it could be the case indeed :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants