Add QueryExecutor trait #338
Conversation
|
All commit authors signed the Contributor License Agreement. |
CodesInChaos
force-pushed
the
query-executor
branch
2 times, most recently
from
cc62585 to
129d062
Compare
The trait enables writing functions that don't care if they're executed in a transaction or directly on a client
CodesInChaos
force-pushed
the
query-executor
branch
from
129d062 to
612599c
Compare
|
This looks good. For reference, postgres crate has similar trait, named |
|
I think we'd need to bump MSRV from 1.72 to 1.75 for " This is a reasonable, since 1.75 is 8 versions before current stable (1.80.1). |
edgedb-tokio/src/query_executor.rs
Outdated
| /// In particular &Client and &mut Transaction | ||
| pub trait QueryExecutor { | ||
| /// see [Client::query] | ||
| fn query<R, A>(self, query: &str, arguments: &A) -> impl Future<Output = Result<Vec<R>, Error>> |
There was a problem hiding this comment.
Handling of
Sendneeds to be discussed. Currently I'm not constraining anything toSend, but perhaps the returned futures need to be, but that might require adding constraints to some of the generic arguments (e.g.QueryResult) as well.
There was a problem hiding this comment.
I think this future should be required to be Send.
Imagine that for some reason, the query results contains a Rc. Imagine that during poll of the future this Rc is somehow mutated & moved across threads. That is not ok.
This is obscure and unlikely to happen and also not a concern of this PR - it should be QueryResult that enforces this constraint.
There was a problem hiding this comment.
I think adding +Sync to the return value requires adding +Sync on QueryResult, or it won't compile. I can easily make that change.
But for some reason neither Client nor Transaction had that constraint on QueryResult (which isn't a problem per-se, since the trait having stricter constraints isn't a problem). Does that mean that Client/Transaction only work on single threaded executors?
But I have little experience with async, so I don't fully understand how Sync interacts with it.
There was a problem hiding this comment.
I think adding
+Sync
You mean Send, right?
Does that mean that Client/Transaction only work on single threaded executors?
No, it means that the future that these function return can be sent between threads. It is a requirement that these function are implemented in a way that can be multi-threaded.
This requirement is a good thing, but it could be too strict for some cases.
There was a problem hiding this comment.
You mean Send, right?
yes
It looks like constraining the Futures in the trait to Send requires constraining QueryResult to be Send as well. I've added those constraints to the PR.
There was a problem hiding this comment.
That's right: for concrete types, Rust compiler can infer if they are Send (and Sync) or not. It's only for generics or dyn types that you need to add an explicit Send.
The trait enables writing functions that don't care if they're executed in a transaction or directly on a client.
Handling of
Sendneeds to be discussed. Currently I'm not constraining anything toSend, but perhaps the returned futures need to be, but that might require adding constraints to some of the generic arguments (e.g.QueryResult) as well.Closes #339