Skip to content

Commit 0daa0fd

Browse files
Copilotjsturtevant
andcommitted
Restore boolean return value from InterruptHandle::kill() with improved docs
Based on feedback, restored the boolean return value for kill() but updated the documentation to clarify what it means: - The boolean indicates whether a signal was sent, not whether cancellation succeeded - On Linux: true if signal was sent to vCPU thread, false if vCPU not running - On Windows: true if WHvCancelRunVirtualProcessor called successfully, false otherwise - A false return doesn't mean failure - the cancellation flag is always set Restored all test assertions that were checking the return value. Co-authored-by: jsturtevant <[email protected]>
1 parent ad4591d commit 0daa0fd

File tree

5 files changed

+31
-23
lines changed

5 files changed

+31
-23
lines changed

docs/cancellation.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ sequenceDiagram
166166
end
167167
168168
deactivate IH
169+
IH-->>Caller: sent_signal
169170
deactivate IH
170171
```
171172

src/hyperlight_host/benches/benchmarks.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ fn bench_guest_call_interrupt_latency(b: &mut criterion::Bencher, size: SandboxS
245245
// Small delay to ensure the guest function is running in VM before interrupting
246246
thread::sleep(std::time::Duration::from_millis(10));
247247
let kill_start = Instant::now();
248-
interrupt_handle.kill();
248+
assert!(interrupt_handle.kill());
249249
kill_start
250250
});
251251

src/hyperlight_host/src/hypervisor/mod.rs

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -209,18 +209,20 @@ pub trait InterruptHandle: Send + Sync + Debug {
209209
/// Interrupt the corresponding sandbox from running.
210210
///
211211
/// This method sets a cancellation flag that prevents or stops the execution of guest code.
212-
/// The effectiveness of this call depends on timing relative to the guest function call lifecycle:
213212
///
214-
/// - **Before guest call starts** (before `clear_cancel()` in `MultiUseSandbox::call()`):
215-
/// The cancellation request will be cleared and ignored.
216-
/// - **After guest call starts but before entering guest code** (after `clear_cancel()`, before `run_vcpu()`):
217-
/// Will prevent the guest from executing.
218-
/// - **While executing guest code**: Will interrupt the vCPU.
219-
/// - **After guest call completes**: Has no effect (cancellation is cleared at the start of the next call).
213+
/// # Return Value
214+
///
215+
/// The return value indicates whether a signal was sent to interrupt a running vCPU:
216+
/// - On Linux: Returns `true` if a signal was sent to the vCPU thread, `false` if the vCPU was not running.
217+
/// - On Windows: Returns `true` if `WHvCancelRunVirtualProcessor` was called successfully, `false` otherwise.
218+
///
219+
/// **Important**: A return value of `false` does not mean the cancellation failed. The cancellation flag is
220+
/// always set, which will prevent or stop execution. A `false` return simply means no signal was sent because
221+
/// the vCPU was not actively running at that moment.
220222
///
221223
/// # Note
222224
/// This function will block for the duration of the time it takes for the vcpu thread to be interrupted.
223-
fn kill(&self);
225+
fn kill(&self) -> bool;
224226

225227
/// Used by a debugger to interrupt the corresponding sandbox from running.
226228
///
@@ -381,13 +383,13 @@ impl InterruptHandleImpl for LinuxInterruptHandle {
381383

382384
#[cfg(any(kvm, mshv3))]
383385
impl InterruptHandle for LinuxInterruptHandle {
384-
fn kill(&self) {
386+
fn kill(&self) -> bool {
385387
// Release ordering ensures that any writes before kill() are visible to the vcpu thread
386388
// when it checks is_cancelled() with Acquire ordering
387389
self.state.fetch_or(Self::CANCEL_BIT, Ordering::Release);
388390

389391
// Send signals to interrupt the vcpu if it's currently running
390-
self.send_signal();
392+
self.send_signal()
391393
}
392394

393395
#[cfg(gdb)]
@@ -520,7 +522,7 @@ impl InterruptHandleImpl for WindowsInterruptHandle {
520522

521523
#[cfg(target_os = "windows")]
522524
impl InterruptHandle for WindowsInterruptHandle {
523-
fn kill(&self) {
525+
fn kill(&self) -> bool {
524526
use windows::Win32::System::Hypervisor::WHvCancelRunVirtualProcessor;
525527

526528
// Release ordering ensures that any writes before kill() are visible to the vcpu thread
@@ -531,7 +533,7 @@ impl InterruptHandle for WindowsInterruptHandle {
531533
// This ensures we see the running state set by the vcpu thread
532534
let state = self.state.load(Ordering::Acquire);
533535
if state & Self::RUNNING_BIT == 0 {
534-
return;
536+
return false;
535537
}
536538

537539
// Take read lock to prevent race with WHvDeletePartition in set_dropped().
@@ -541,19 +543,23 @@ impl InterruptHandle for WindowsInterruptHandle {
541543
Ok(guard) => guard,
542544
Err(e) => {
543545
log::error!("Failed to acquire partition_state read lock: {}", e);
544-
return;
546+
return false;
545547
}
546548
};
547549

548550
if guard.dropped {
549-
return;
551+
return false;
550552
}
551553

552554
unsafe {
553-
if let Err(e) = WHvCancelRunVirtualProcessor(guard.handle, 0, 0) {
554-
log::error!("Failed to cancel running virtual processor: {}", e);
555+
match WHvCancelRunVirtualProcessor(guard.handle, 0, 0) {
556+
Ok(_) => true,
557+
Err(e) => {
558+
log::error!("Failed to cancel running virtual processor: {}", e);
559+
false
560+
}
555561
}
556-
};
562+
}
557563
}
558564
#[cfg(gdb)]
559565
fn kill_from_debugger(&self) -> bool {

src/hyperlight_host/src/metrics/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ mod tests {
118118
// interrupt the guest function call to "Spin" after 1 second
119119
let thread = thread::spawn(move || {
120120
thread::sleep(Duration::from_secs(1));
121-
interrupt_handle.kill();
121+
assert!(interrupt_handle.kill());
122122
});
123123

124124
multi

src/hyperlight_host/tests/integration_test.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ fn interrupt_in_progress_guest_call() {
8787
// kill vm after 1 second
8888
let thread = thread::spawn(move || {
8989
thread::sleep(Duration::from_secs(1));
90-
interrupt_handle.kill();
90+
assert!(interrupt_handle.kill());
9191
barrier2.wait(); // wait here until main thread has returned from the interrupted guest call
9292
barrier2.wait(); // wait here until main thread has dropped the sandbox
9393
assert!(interrupt_handle.dropped());
@@ -122,7 +122,7 @@ fn interrupt_guest_call_in_advance() {
122122

123123
// kill vm before the guest call has started
124124
let thread = thread::spawn(move || {
125-
interrupt_handle.kill();
125+
assert!(!interrupt_handle.kill()); // should return false since vcpu is not running yet
126126
barrier2.wait();
127127
barrier2.wait(); // wait here until main thread has dropped the sandbox
128128
assert!(interrupt_handle.dropped());
@@ -274,9 +274,10 @@ fn interrupt_moved_sandbox() {
274274
let thread2 = thread::spawn(move || {
275275
barrier.wait();
276276
thread::sleep(Duration::from_secs(1));
277-
interrupt_handle.kill();
277+
assert!(interrupt_handle.kill());
278278

279-
interrupt_handle2.kill();
279+
// make sure this returns true, which means the sandbox wasn't killed incorrectly before
280+
assert!(interrupt_handle2.kill());
280281
});
281282

282283
let res = sbox2.call::<i32>("Spin", ()).unwrap_err();

0 commit comments

Comments
 (0)