Make API server database 100% async #1195
Conversation
TheQuantumPhysicist
requested review from
muursh,
erubboli,
iljakuklic and
azarovh
as code owners
| .map_err(|e| ApiServerStorageError::LowLevelStorageError(e.to_string()))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn create_tables(&mut self) -> Result<(), ApiServerStorageError> { | ||
| pub async fn create_tables(&mut self) -> Result<(), ApiServerStorageError> { | ||
| logging::log::info!("Creating database tables"); | ||
|
|
||
| self.just_execute( | ||
| "CREATE TABLE ml_misc_data ( | ||
| name TEXT PRIMARY KEY, | ||
| value BLOB NOT NULL |
There was a problem hiding this comment.
I might change these BLOBs to bytea or TEXT
There was a problem hiding this comment.
Feel free to use whatever works for you.
| fn drop(&mut self) { | ||
| if !self.finished { | ||
| futures::executor::block_on(self.connection.batch_execute("ROLLBACK")).unwrap_or_else( | ||
| |e| { | ||
| logging::log::error!( | ||
| "CRITICAL ERROR: failed to rollback failed postgres RW transaction: {e}" | ||
| ) | ||
| }, | ||
| ); | ||
| } | ||
| .try_build()?; | ||
| } |
There was a problem hiding this comment.
I am not sure about all the implications of block_on in the destructor, seems like it could lead to problems if the future needs something else to resolve as part of its execution. Also, if I understand this correctly, the executor from the futures crate is different from the tokio one. Is it fine to mix and match them?
Not sure what to do about it. One option is to enforce that the user uses explicitly one of the commit or rollback methods to end the transaction. In the destructor, we just assert self.finished. However, it may be hard to chase up all the places where a transaction object is destroyed, especially considering all the ?s and other forms of early return.
There was a problem hiding this comment.
Is there a test that exercises this code path?
There was a problem hiding this comment.
There's zero tests for postgres yet. Alfie is working on the containerization.
I've been thinking about this block_on problem yesterday all night (and panicking when forgetting occurred to me, but it's ugly)... I have an idea on how to get rid of it here... I'll discuss it in the daily.
There was a problem hiding this comment.
All my attempts to move the processing to the db as a parent failed so far. There's always the requirement to make something here or there 'static. The closest I came to succeeding is to make channels that send tasks to a tokio task, but then because the connection has a lifetime, it's mandating having 'static lifetime for the receiver.
scoped tokio didn't help because it returns a reference that cannot be moved out of scope.
Same for std scoped threads, because the scope cannot be moved.
No idea how I'll proceed. Maybe I'll have the patience to do it later. Feel free to take a poke at it.
The only problem left that I don't know how to fix is how to make transaction drops async too... but I guess this is a problem in rust not support AsyncDrop yet... but otherwise, I think this is a viable, working code.