Conversation
af5f94e to
901c24e
Compare
crates/sqlite-inproc/src/lib.rs
Outdated
| if cols_tx.send(columns).is_err() { | ||
| _ = err_tx.send(Err(v3::Error::Io("cols send failed".into()))); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If you'd like one way to make this cleaner would be to separate this as:
let the_actual_work = async move {
// ... code using `?` and such ...
Ok(())
};
tokio::spawn(async move {
if let Err(e) = the_actual_work.await {
err_tx.send(e);
}
})basically re-enable using ? idiomatically without the worry of having to send an error and early-return every time
crates/key-value-spin/src/store.rs
Outdated
| let connection = self.connection.clone(); | ||
| let name = self.name.clone(); | ||
|
|
||
| tokio::spawn(async move { |
There was a problem hiding this comment.
This may be preexisting and/or my own ignorance, but I was surprised by the lack of await in the task below. Is the underlying connection not actually async?
There was a problem hiding this comment.
The backing store is SQLite, and as far as I can tell rusqlite doesn't provide an async API for that.
There was a problem hiding this comment.
Ah ok, in that case you'll want to use tokio::spawn_blocking here to signify that this is doing blocking I/O and shouldn't be run on the main event loop
crates/wasi-async/src/stream.rs
Outdated
|
|
||
| tokio::spawn(async move { | ||
| loop { | ||
| match sync_rx.recv_timeout(tokio::time::Duration::from_millis(1)) { |
There was a problem hiding this comment.
If possible I'd avoid having this function where possible, blocking the event loop even for 1ms can have some bad side effects. Could the callers be switched to already using tokio::sync::mpsc instead?
There was a problem hiding this comment.
The trouble I ran into with this was that the SQLite types involved are not Send. So await-ing across a Tokio Sender::send gave me a sea of red - hence the act of desperation of using a std::sync channel. Perhaps we could zoosh the async send off to a separate task or something? (So that we can await it without having an await in the main loop.) By that point the sendee is owned, so I guess something like this should work...?
There was a problem hiding this comment.
Huh okay that doesn't look like it will work easily. Is a viable alternative to use tokio::task::spawn_blocking in the asyncify function?
There was a problem hiding this comment.
Ah for that yeah I think you'll spawn_blocking the main loop that processes for sqlite and you won't use .await internally. To send values you'll want blocking_send. That way everything's using Tokio types but it's also using blocking functions for the spawn_blocking closure doing I/O
901c24e to
6253048
Compare
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
6253048 to
e9c783d
Compare
This currently includes the PostgreSQL async because it uses some common stuff from that PR. Please ignore that stuff in review (we can either make this a roll-up PR or I can rebase it, whatever suits).Current status:
untested, just getting it into the systemtested the builtin (local SQLite) implementations and they Worked On My Machine TM. Well, they work now (cough, cough)ETA: tried it with Redis-backed KV and libsql-backed SQLite (using Turso localdev). And... it worked? Conclusion: I am testing it wrong.
I am not sure how to test the Azure and AWS KV back-ends.
A word on how I am revving packages:
spin:namespace, I'm adding async functions to the existing interface via a minor version rev, using thefoo-asyncnaming convention.fermyon:namespace, I'm creating a new package in thespin:namespace. Since the new package is ALL ASYNC ALL THE TIME - it doesn't come with parallel sync-async versions - I'm reusing the existing function names, without the-asyncdecoration.