cksac/dataloader-rs

non-cached `try_load` panics instead of returning `Err` when requested by more than one

6293 opened this issue · 0 comments

6293 commented

this test fails by panicking when I add requesters like below:

let load_fn = LoadFnForEmptyTest;
let loader = Loader::new(load_fn.clone()).with_max_batch_size(4);

let l1 = loader.clone();
let h1 = thread::spawn(move || {
    let r1 = l1.try_load(1337);
    let r2 = l1.try_load(1338);

    let (f1, f2) = block_on(futures::future::join(r1, r2));
    assert!(f1.is_err());
    assert!(f2.is_err());
});

let _ = h1.join().unwrap();

This is because this line expects value to be present at state.complete, while early-returning here may make values to be discarded (this is why the test passes when there is only one requester). Possible fix would be to put error handling after batch completion, along with failed state which would hold <requestId, key> (without introducing this state, we would have no way to attach key to error message as now)