m1sterc001guy
commented
1 year ago Second, split the commit_tx into trait IDatabaseTransaction : IDatabaseTransactionRef. There are some places where we explicitily don't want the user of DatabaseTransaction (module code) to be allowed to call commit_tx so this is probably necessary as well. Maybe just doing this part will make the first one unnecessary.
If we do this, I think we need to introduce a new wrapper type. Right now, both "Isolated" db transactions and "Commitable" transactions are the same wrapper type (DatabaseTransaction). Isolated transactions just panic if commit_tx is called. If we want to prevent the modules from ever committing, I think we'd need a new wrapper type that didn't have commit_tx as a function option.
dpc
maan2003
I remember there were some patterns that were not possible due to that (maybe @Maan2003 remembers).
Two possible (somewhat orthogonal) things we could do:
First, make
commit_txtake&mut selfin the trait, but keep itselfon theDatabaseTransactionnewtype wrapper.This would allow us to always wrap inner
IDatabaseTranasctionvia&mut dyn IDatabaseTranasction, avoiding allocations. Under the hood the implementations often need to consume the transaction (that's why we madecommit_txtakeselfby move. But they could useOptionand panic if called twice (the dyn newtype will ensure this can't happen anyway), or even usemem::replaceto replace the old tx with a new one (if this doesn't cost us any performance)Second, split the
commit_txintotrait IDatabaseTransaction : IDatabaseTransactionRef. There are some places where we explicitily don't want the user ofDatabaseTransaction(module code) to be allowed to callcommit_txso this is probably necessary as well. Maybe just doing this part will make the first one unnecessary.