Reset db connection before returning it to the pool

[jellonek]

Motivation

We have seen in logs:

http: panic serving 127.0.0.1:34652: connection returned to pool has active statement: "select address, balance, next_nonce, max(layer_updated), template, state from accounts group by address order by layer_updated desc limit 100"
as part of http handler recovery function is returning connection to the pool, while it can have pending, non finished statement.

Code in this patch should fix this issue.

Test Plan

No new unittests added as the original issue was not reproduced.

TODO

• Explain motivation or link existing issue(s)
• Test changes and document test plan
• Update documentation as needed
• Update changelog as needed

[ivan4th]

Maybe putConn given that we have getConn already and we're dealing with a pool with its "get"/"put" operations?
Also returnConn sounds a bit misleading as it may sound as if it's dealing with return value of some kind

[jellonek]

In IT we have 2 main issues - naming and cache invalidation ;)
I was thinking too long on how to name this func :D but i'll take your suggestion.

[ivan4th]

This probably needs some comments explaining why it needs to happen this way

[jellonek]

Comment with explanation added.

[fasmat]

Can you add some tests that fail without your change but pass with the adapted code?

[fasmat]

Also: this feels like a hack that is necessary, because we might not be using contexts correctly in database.go.

For instance: I feel like withTx should look like this:

func (db *sqliteDatabase) withTx(ctx context.Context, initstmt string, exec func(Transaction) error) (err error) {
	txCtx, cancel := context.WithCancel()
	defer cancel()
	tx, err := db.getTx(txCtx, initstmt)
	if err != nil {
		return err
	}
	defer func() {
		if rErr := tx.Release(); rErr != nil && err == nil {
			err = fmt.Errorf("release tx: %w", rErr)
		}
	}()
	if err := exec(tx); err != nil {
		tx.queryCache.ClearCache()
		return err
	}
	return tx.Commit()
}

If now the exec function panics the connection should be returned to the pool as expected, even if the caller passes context.Background into WithTx without having to adjust tx.Release().

[fasmat]

I just thought of a better approach than my previous suggestion: withTx doesn't need to be modified at all if getTx and sqliteTx are adapted:

func (db *sqliteDatabase) getTx(ctx context.Context, initstmt string) (*sqliteTx, error) {
	if db.closed {
		return nil, ErrClosed
	}
	conCtx, cancel := context.WithCancel(ctx)
	conn := db.getConn(conCtx)
	if conn == nil {
		cancel()
		return nil, ErrNoConnection
	}
	tx := &sqliteTx{queryCache: db.queryCache, db: db, conn: conn, freeConn: cancel}
	if err := tx.begin(initstmt); err != nil {
		cancel()
		// TODO: shouldn't the TX also be put back into the pool here?
		return nil, err
	}
	return tx, nil
}

// sqliteTx is wrapper for database transaction.
type sqliteTx struct {
	*queryCache

	db        *sqliteDatabase
	conn      *sqlite.Conn
	freeConn  func()
	committed bool
	err       error
}

// Release transaction. Every transaction that was created must be released.
func (tx *sqliteTx) Release() error {
	tx.freeConn()
	defer tx.db.pool.Put(tx.conn)
	if tx.committed {
		return nil
	}
	stmt := tx.conn.Prep("ROLLBACK")
	_, tx.err = stmt.Step()
	return mapSqliteError(tx.err)
}

[codecov]

Codecov Report

Attention: Patch coverage is 38.88889% with 11 lines in your changes missing coverage. Please review.

Project coverage is 79.7%. Comparing base (55c3547) to head (dc2f28e).

Files with missing lines	Patch %	Lines
sql/database.go	38.8%	10 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6407     +/-   ##
=========================================
- Coverage     79.8%   79.7%   -0.1%     
=========================================
  Files          328     328             
  Lines        42879   42893     +14     
=========================================
- Hits         34221   34201     -20     
- Misses        6725    6758     +33     
- Partials      1933    1934      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jellonek]

IMO your proposed code will fail as cancel func is nowhere passed so it can not be called when needed - before returning connection to the pool.
IMO that would work if we would store cancel func on db struct then call it before calling db.pool.Put() in any defer.

[jellonek]

Closing in favor of more clean approach proposed in #6408