Keep sea-orm-sync's DatabaseConnection Send + Sync (#3070) - #3166
Open
teddytennant wants to merge 1 commit into
Open
Keep sea-orm-sync's DatabaseConnection Send + Sync (#3070)#3166teddytennant wants to merge 1 commit into
teddytennant wants to merge 1 commit into
Conversation
make-sync.sh strips the Send and Sync bounds from every trait bound in src/ so that the generated crate carries no executor bounds. The strip is indiscriminate, so it also removed the bounds from two trait objects that are stored as plain data inside DatabaseConnection: the metric callback (Arc<dyn Fn(&Info<'_>)>) and MockDatabaseTrait. That left sea-orm-sync's DatabaseConnection neither Send nor Sync, so it cannot be put in a static OnceLock, even though the connection it wraps is already an Arc<Mutex<State>>. Restore both bounds after the blanket strip, alongside the existing Arc<dyn std::error::Error> restore, and drop the cfg gate that excluded sea-orm-sync from the DatabaseConnection trait assertion. RusqliteSharedConnection::set_metric_callback was missing the bound in the async source too, unlike its three sqlx siblings; it is never type-checked there because the root crate cannot build with --features rusqlite.
Author
|
Thanks. I did use an LLM on this PR (mostly on the writeup and while Say if you want the description shortened. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3070.
The bug
In
sea-orm-sync,DatabaseConnectionis neitherSendnorSync, so it cannot be stored in a static:This is not a property of the sync design —
RusqliteSharedConnectionalready holds its connection in anArc<Mutex<State>>, andmake-sync.shdeliberately mapsfutures_util::lock::Mutextostd::sync::Mutex. The only things standing in the way are two trait objects that lost their bounds during generation.Root cause
make-sync.shstripsSend/Syncfrom every bound insrc/:That is right for future/executor bounds, but the strip is indiscriminate and also hits two trait objects that are plain data stored inside
DatabaseConnection:type Callback = Arc<dyn Fn(&Info<'_>) + Send + Sync>type Callback = Arc<dyn Fn(&Info<'_>)>pub trait MockDatabaseTrait: Send + Debugpub trait MockDatabaseTrait: DebugNeither has anything to do with async. The script already recognises this class of over-strip and repairs it one line below, for the error type:
replace_rs 's/Arc<dyn std::error::Error>/Arc<dyn std::error::Error + Send + Sync>/' src@Huliiiiii diagnosed this on the issue the day it was filed ("make sync script lacks coverage") — this PR fixes it in the generator rather than hand-editing
sea-orm-sync/, since that directory is generated output.The fix
build-tools/make-sync.sh— three restores placed next to the existingArc<dyn std::error::Error>one: themetric::Callbackalias, theset_metric_callbackFbound it is constructed from, andMockDatabaseTrait.sea-orm-sync/is then regenerated; the regeneration is idempotent (running the script twice produces a byte-identical tree, and running it onmasterbefore this change is a no-op).src/driver/rusqlite.rs—RusqliteSharedConnection::set_metric_callbackwas declared asF: Fn(&Info<'_>) + 'static, while the three sqlx drivers all useF: Fn(&Info<'_>) + Send + Sync + 'static. The async crate never catches this because the root crate cannot build with--features rusqlite(it needssea_query_rusqlite, which is only wired up insea-orm-sync/Cargo.toml). Aligned with its siblings.Test
The repo already pins this invariant in
src/database/db_connection.rs, but with#[cfg(not(feature = "sync"))]so it never ran for the sync crate. The gate is removed so the assertion covers both crates. It is written as two single-bound helpers rather than oneassert_send_syncbecausemake-sync.shwould strip a multi-bound clause.Before (
cd sea-orm-sync && cargo test --lib --features rusqlite, with the test present but the generator unfixed):After:
The reporter's original snippet now compiles against
sea-orm-syncas well.Also run:
cd sea-orm-sync && DATABASE_URL="sqlite::memory:" cargo test --test '*' --features tests-features,rusqlite— all suites green (therusqliteCI job's command)cargo test --libon the async crate — 249 passedcargo fmt --all -- --check(nightly) and the same forsea-orm-sync— cleancargo clippy --all -- -D warningsandcargo clippy --all --features runtime-tokio-native-tls,sqlx-all -- -D warnings— cleanI checked that a partial fix is still caught: restoring only
Sendon the callback alias, or dropping only theMockDatabaseTraitrestore, both leave the assertion failing to compile.Deliberately not changed
sea-orm-sync/src/— everything there in this diff is generator output.MockDatabaseTraitgainingSendis a bound tightening forsea-orm-syncimplementors, but it only restores parity withsea-orm, where the trait has always requiredSend.rusqliteCI job runscargo test --test '*', so it does not execute lib unit tests and will not enforce this assertion forsea-orm-sync(same gap noted in Fix sync-variant futures_util handling; regenerate sea-orm-sync #3112). Happy to add--libto that job if you want it enforced; I left CI alone to keep the diff focused.