Skip to content
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

Fix context cancellation handling in the database code #6273

Open
ivan4th opened this issue Aug 20, 2024 · 1 comment
Open

Fix context cancellation handling in the database code #6273

ivan4th opened this issue Aug 20, 2024 · 1 comment

Comments

@ivan4th
Copy link
Contributor

ivan4th commented Aug 20, 2024

Description

In order to detect the situation when a context used to create an SQLite transaction is canceled, the database code checks for sqlite.SQLITE_INTERRUPT error and assumes it always means context cancellation. The error is returned due to this mechanism: https://github.com/go-llsqlite/crawshaw/blob/v0.5.3/sqlitex/pool.go#L177

Another issue is that interrupting any writes may cause deadlocks: #5716
We either need to find a way to fix it or disallow using interruptible contexts for any database writes.
That is, if you create a transaction with a Context passed, the transaction must be effectively read-only, e.g. any Execs should fail upon UPDATE/INSERT/DELETE/DROP statements. This may require some further consideration

Also, we need to check whether crawshaw actually frees the connections for which the context is canceled.

Additional notes:

Ideally need to have 2 connection pools internally in the sql.Database, one of which is read-only and the other one allows writes. The write-enabled pool should have just one (1) connection available. Also, only the transactions created for the read-only pool should be cancelable and the corresponding WithWriteTx(...) etc. methods should not accept a context.

The read-only pool should be used for everything except write transactions.

@fasmat
Copy link
Member

fasmat commented Aug 20, 2024

As stated in the discussion on the #6003, maybe we can tie transaction release / rollback to the context cancellation with context.AfterFunc: #6003 (comment)

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

No branches or pull requests

2 participants