[wgpu-core] simplify callbacks #6624
Conversation
teoxoy
force-pushed
the
device-lost-cb
branch
4 times, most recently
from
871cd15 to
9d67652
Compare
| #[gpu_test] | ||
| static DEVICE_DROP_THEN_LOST: GpuTestConfiguration = GpuTestConfiguration::new() | ||
| .parameters(TestParameters::default().expect_fail(FailureCase::webgl2())) | ||
| .run_sync(|ctx| { | ||
| // This test checks that when the device is dropped (such as in a GC), | ||
| // the provided DeviceLostClosure is called with reason DeviceLostReason::Dropped. | ||
| // Fails on webgl because webgl doesn't implement drop. | ||
| static WAS_CALLED: std::sync::atomic::AtomicBool = AtomicBool::new(false); | ||
|
|
||
| // Set a LoseDeviceCallback on the device. | ||
| let callback = Box::new(|reason, message| { | ||
| WAS_CALLED.store(true, std::sync::atomic::Ordering::SeqCst); | ||
| assert_eq!(reason, wgt::DeviceLostReason::Dropped); | ||
| assert_eq!(message, "Device dropped."); | ||
| }); | ||
| ctx.device.set_device_lost_callback(callback); | ||
|
|
||
| drop(ctx); | ||
|
|
||
| assert!( | ||
| WAS_CALLED.load(std::sync::atomic::Ordering::SeqCst), | ||
| "Device lost callback should have been called." | ||
| ); | ||
| }); | ||
|
|
||
| #[gpu_test] | ||
| static DEVICE_LOST_REPLACED_CALLBACK: GpuTestConfiguration = GpuTestConfiguration::new() | ||
| .parameters(TestParameters::default()) | ||
| .run_sync(|ctx| { | ||
| // This test checks that a device_lost_callback is called when it is | ||
| // replaced by another callback. | ||
| static WAS_CALLED: AtomicBool = AtomicBool::new(false); | ||
|
|
||
| // Set a LoseDeviceCallback on the device. | ||
| let callback = Box::new(|reason, _m| { | ||
| WAS_CALLED.store(true, std::sync::atomic::Ordering::SeqCst); | ||
| assert_eq!(reason, wgt::DeviceLostReason::ReplacedCallback); | ||
| }); | ||
| ctx.device.set_device_lost_callback(callback); | ||
|
|
||
| // Replace the callback. | ||
| let replacement_callback = Box::new(move |_r, _m| {}); | ||
| ctx.device.set_device_lost_callback(replacement_callback); | ||
|
|
||
| assert!( | ||
| WAS_CALLED.load(std::sync::atomic::Ordering::SeqCst), | ||
| "Device lost callback should have been called." | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
suggestion: I actually thought it might be valuable to take these tests and make them assert the opposite of what they originally did; that is, we guarantee that these are not called. in between.
Happy to push a fixup! if you agree.
There was a problem hiding this comment.
I don't think we can guarantee that though. Technically device loss could happen at any time.
There was a problem hiding this comment.
I mean, technically yes, our API needs to handle that at any given time, but we do control the testing environment fairly well, and we're not really exercising any Device functionality in these tests. Are you worried about intermittent test failures still happening, despite all that?
I think if we completely drop the ctx before the check, that makes the test interesting enough. I really can only see such intermittencies happening often in the face of other problems that would make other tests fail, anyway.
There was a problem hiding this comment.
That's fair but I'm not sure it's worth testing that something doesn't happen if the code that was triggering the behavior is gone.
ErichDonGubler
added
type: enhancement
teoxoy
force-pushed
the
device-lost-cb
branch
from
9d67652 to
c0d3fd1
Compare
Follow-up to #6588.
At a high-level:
wgpu-corealready use the rust variants (incudingwgpu-native).device_unregister_device_lost_closureas users of the API that have cleanup work to do can just implementDropfor state passed into the closure