already borrowed: BorrowMutError
#205
Comments
|
Sorry for the bug, and thanks for narrowing it down and sending the stack trace — very helpful! It's currently late in the evening over here, but I'll look into this first thing tomorrow. At first glance, it looks like it shouldn't be a difficult fix and I'll get you unblocked ASAP. |
|
@obi1kenobi No rush, I am just prototyping at the moment, so it isn't a blocker. |
|
Interestingly, I'm not able to reproduce this error using the test schemas and adapters in the test suite. I've tried a variety of queries This is probably at least a contributing factor if not the outright reason why fuzzing didn't find the problem either. Depending on your preference, there are two things we can do:
Sorry for the inconvenience! Let me know your thoughts. |
|
I also forgot to ask: did you write your adapter in Rust or are you using the Python or WASM bindings instead? |
|
Thanks for looking into it @obi1kenobi The adapter is in Rust. I'm off on leave for a few weeks, when I get back I'll see if I can get a minimal reproducible example. It could be that I am misusing the adapter API. |
|
Thanks! I'm pretty sure this is a bug on the library side — no matter how the adapter is (mis)used, it shouldn't ever trigger a double borrow error like this. I'd love to track it down, and I appreciate your help. No rush, whenever you're able to get back to it is fine. I'll set a reminder to check back in in a month if I haven't heard back yet. |
|
Hi @obi1kenobi. I have managed to create a minimal reproducible example which breaks on my machine. I also tracked down the culprit, there is a borrow in Breaking Exampleuse std::cell::RefCell;
use std::{collections::BTreeMap, fs, iter::*, rc::Rc};
use trustfall::{provider::*, FieldValue, Schema};
fn main() {
let query = QUERY;
let schema = Schema::parse(SCHEMA).unwrap();
let adptr = Rc::new(RefCell::new(BulkAdapter {}));
let variables: BTreeMap<String, FieldValue> = Default::default();
let xs = trustfall::execute_query(&schema, Rc::clone(&adptr), query, variables).unwrap();
}
pub struct BulkAdapter {}
#[derive(Debug, Clone, TrustfallEnumVertex)]
pub enum Db {
Spry,
}
impl<'vertex> BasicAdapter<'vertex> for BulkAdapter {
type Vertex = Db;
fn resolve_starting_vertices(
&mut self,
edge_name: &str,
parameters: &EdgeParameters,
) -> VertexIterator<'vertex, Self::Vertex> {
Box::new(once(Db::Spry))
}
fn resolve_property(
&mut self,
contexts: ContextIterator<'vertex, Self::Vertex>,
type_name: &str,
property_name: &str,
) -> ContextOutcomeIterator<'vertex, Self::Vertex, trustfall::FieldValue> {
let contexts = contexts.map(|x| dbg!(x)).collect::<Vec<_>>();
todo!()
}
fn resolve_neighbors(
&mut self,
contexts: ContextIterator<'vertex, Self::Vertex>,
type_name: &str,
edge_name: &str,
parameters: &EdgeParameters,
) -> ContextOutcomeIterator<'vertex, Self::Vertex, VertexIterator<'vertex, Self::Vertex>> {
trustfall::provider::resolve_neighbors_with(contexts, |_| Box::new(once(Db::Spry)))
}
fn resolve_coercion(
&mut self,
contexts: ContextIterator<'vertex, Self::Vertex>,
type_name: &str,
coerce_to_type: &str,
) -> ContextOutcomeIterator<'vertex, Self::Vertex, bool> {
todo!()
}
}
const QUERY: &str = r#"
# import * from "src/schema.graphql"
{
Spry(file: "dev.spry") {
cases {
# fullname @output
name @output
equipment @fold {
eqname: fullname @output
}
# steps {
# index @output
# }
}
}
}
"#;
const SCHEMA: &str = r#"
schema {
query: RootSchemaQuery
}
directive @output(
"""What to designate the output field generated from this property field."""
name: String
) on FIELD
directive @fold on FIELD
type RootSchemaQuery {
Spry(file: String): SpryProject
}
type SpryProject {
cases: [Case]
}
type Case {
name: String
fullname: String
equipment: [Equipment]
steps: [ScheduleStep]
}
type Equipment {
name: String
fullname: String
}
interface ScheduleStep {
index: Int
}
type ProductiveScheduleStep implements ScheduleStep {
index: Int
}
"#;More Details of Borrow Stack
trustfall/trustfall_core/src/interpreter/execution.rs Lines 69 to 71 in db8abec // The `compute_fold` is _lazily_ being evaluated here
iterator = compute_component(adapter.clone(), &mut carrier, component, iterator);
// passed here
Ok(construct_outputs(adapter.as_ref(), &mut carrier, iterator))
trustfall/trustfall_core/src/interpreter/execution.rs Lines 238 to 253 in db8abec // This iterator is still lazy
let moved_iterator = Box::new(output_iterator.map(move |context| {
let new_vertex = context.vertices[&vertex_id].clone();
context.move_to_vertex(new_vertex)
}));
// NOTE: if we append a `.collect::<Vec<_>>()` after the map this will
// avoid the double borrow because the iteration will have evaluated.
// borrow occurs here
let mut adapter_ref = adapter.borrow_mut();
let resolve_info = ResolveInfo::new(query, vertex_id, true);
let type_name = &root_component.vertices[&vertex_id].type_name;
// The moved iterator will invoke `compute_fold` when evaluating.
let field_data_iterator = adapter_ref.resolve_property(
moved_iterator,
type_name,
&context_field.field_name,
&resolve_info,
);
drop(adapter_ref); |
|
Thank you so much for all the work you've done on this! I'm making this issue my top priority for today. |
|
Fascinating. Confirming this as a re-entrancy bug. If it wasn't for Rust's commitment to safety, this would have been ~impossible to get to the bottom of, since the symptoms would have started even more downstream from here. Collecting the iterator early would solve it, but it is quite undesirable as a solution since it makes almost the entire execution eager instead of lazy. I'm going to spend some time looking for alternatives. Another possible workaround for your particular adapter is to avoid the Again, apologies for the inconvenience and thanks for the help in getting to the bottom of it! |
|
For the benefit of both the current implementation and future work like #83, I think the best way to fix this issue is with a breaking change to the I'm planning to change the Unfortunately, it also means that users that need mutability will need to use their own |
|
#249 has the fix. It needs a tiny amount of additional work before I can merge it. I'm planning to release it as |
|
I just queued up the release of There's a test case for the exact situation in your repro example, so the issue should be solved and won't be coming back. Thanks again for reporting the problem and helping track it down! |
|
Thanks for the speedy fix! |
|
My pleasure! Thanks for the help!
…On Sat, Apr 8, 2023, 5:38 PM Kurt Lawrence ***@***.***> wrote:
Thanks for the speedy fix!
—
Reply to this email directly, view it on GitHub
<#205 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAR5MSTBL2DPKAUAJY2JOTDXAHLEDANCNFSM6AAAAAAVM2W6EU>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
I am getting a 'already borrowed' error when using
@foldadjacent to other output fields.This query would break:
{ Spry(file: "dev.spry") { cases { name @output fullname @output equipment @fold { eqname: name @output eqname_full: fullname @output } } } }But without the adjacent fields, it returns as expected.
{ Spry(file: "dev.spry") { cases { equipment @fold { eqname: name @output eqname_full: fullname @output } } } }Stack Trace
The text was updated successfully, but these errors were encountered: