From e31008db14054cc884ec2f8f4f7f5a07872fa398 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Thu, 18 May 2023 22:09:28 +0300 Subject: fix(tvix/store): fix timing sensitivity in gRPC directorysvc test One test in here assumed the server was fast, but when very busy, a computer running these tests might not be fast. Treat both cases that can occur separately. Change-Id: Iba168ad39f2458c4fb8873211df33beeaff7c6c1 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8595 Reviewed-by: tazjin Tested-by: BuildkiteCI --- tvix/store/src/directoryservice/grpc.rs | 69 +++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 24 deletions(-) (limited to 'tvix/store/src/directoryservice/grpc.rs') diff --git a/tvix/store/src/directoryservice/grpc.rs b/tvix/store/src/directoryservice/grpc.rs index 3c76d9e315..d4ac9fd925 100644 --- a/tvix/store/src/directoryservice/grpc.rs +++ b/tvix/store/src/directoryservice/grpc.rs @@ -434,6 +434,14 @@ mod tests { .put(DIRECTORY_B.clone()) .expect_err("must fail"); + // Putting DIRECTORY_B in a put_multiple will succeed, but the close + // will always fail. + { + let mut handle = directory_service.put_multiple_start(); + handle.put(DIRECTORY_B.clone()).expect("must succeed"); + handle.close().expect_err("must fail"); + } + // Uploading A and then B should succeed, and closing should return the digest of B. let mut handle = directory_service.put_multiple_start(); handle.put(DIRECTORY_A.clone()).expect("must succeed"); @@ -458,36 +466,49 @@ mod tests { .expect("must succeed") ); - // Uploading B and then A should fail during close (if we're a - // fast client) - let mut handle = directory_service.put_multiple_start(); - handle.put(DIRECTORY_B.clone()).expect("must succeed"); - handle.put(DIRECTORY_A.clone()).expect("must succeed"); - handle.close().expect_err("must fail"); + // Uploading B and then A should fail, because B refers to A, which + // hasn't been uploaded yet. + // However, the client can burst, so we might not have received the + // error back from the server. + { + let mut handle = directory_service.put_multiple_start(); + // sending out B will always be fine + handle.put(DIRECTORY_B.clone()).expect("must succeed"); + + // whether we will be able to put A as well depends on whether we + // already received the error about B. + if handle.put(DIRECTORY_A.clone()).is_ok() { + // If we didn't, and this was Ok(_), … + // a subsequent close MUST fail (because it waits for the + // server) + handle.close().expect_err("must fail"); + } + } - // Below test is a bit timing sensitive. We send B (which refers to - // A, so should fail), and wait sufficiently enough for the server + // Now we do the same test as before, send B, then A, but wait + // sufficiently enough for the server to have s // to close us the stream, // and then assert that uploading anything else via the handle will fail. - - let mut handle = directory_service.put_multiple_start(); - handle.put(DIRECTORY_B.clone()).expect("must succeed"); - - let mut is_closed = false; - for _try in 1..20 { - if handle.is_closed() { - is_closed = true; - break; + { + let mut handle = directory_service.put_multiple_start(); + handle.put(DIRECTORY_B.clone()).expect("must succeed"); + + let mut is_closed = false; + for _try in 1..1000 { + if handle.is_closed() { + is_closed = true; + break; + } + std::thread::sleep(time::Duration::from_millis(10)) } - std::thread::sleep(time::Duration::from_millis(200)) - } - assert!( - is_closed, - "expected channel to eventually close, but never happened" - ); + assert!( + is_closed, + "expected channel to eventually close, but never happened" + ); - handle.put(DIRECTORY_A.clone()).expect_err("must fail"); + handle.put(DIRECTORY_A.clone()).expect_err("must fail"); + } }); tester_runtime.block_on(task)?; -- cgit 1.4.1