Conversation
wdcui
left a comment
wdcui
left a comment
There was a problem hiding this comment.
LGTM. I left a minor comment below.
| // `TimerHandle`. The handle's `Drop` impl waits for all in-flight | ||
| // callbacks before dropping the context, so this reference is valid. | ||
| let ctx = unsafe { &*context.cast::<TimerCallbackContext>() }; | ||
| let thread = ACTIVE_THREADS.lock().unwrap().first().cloned(); |
There was a problem hiding this comment.
I got the following comment from GH Copilot. Is this a valid concern? If so, we may want to add a note about it the comments.
"The one thing to watch is that ACTIVE_THREADS.lock().unwrap().first() always picks the same thread. If you ever support multiple guest threads that each need independent timer delivery, you may want to associate the timer with a specific thread rather than broadcasting to the first one in the list. But for single-process/single-main-thread use, the current approach is fine."
| /// Timer support for proactive signal delivery. | ||
| /// | ||
| /// Platforms that support this should set [`SUPPORTS_TIMER`](Self::SUPPORTS_TIMER) | ||
| /// to `true`. | ||
| pub trait TimerProvider { | ||
| /// The platform-specific timer handle type. | ||
| type TimerHandle: TimerHandle; | ||
|
|
||
| /// Whether this platform supports [`TimerProvider`] for proactive timer delivery. | ||
| const SUPPORTS_TIMER: bool = false; | ||
|
|
||
| /// Create a new one-shot timer that delivers `signal` when it fires. | ||
| fn create_timer(&self, signal: crate::shim::Signal) -> Self::TimerHandle; | ||
| } |
There was a problem hiding this comment.
Minor:
I'm not sure SUPPORTS_TIMER is the ideal move, a possible alternative is to do
pub trait TimerProvider {
type TimerHandle: TimerHandle; // Remind users to use `trivial_providers::UnsupportedTimerHandle` if not supported
fn create_timer(&self, signal: Signal) -> Option<Self::TimerHandle> { None }
}
UnsupportedTimerHandle should be defined as pub enum UnsupportedTimerHandle {} which is equivalent to Rust !, which means that it can never actually be created; that way you are guaranteeing it via the type system.
| /// Dropping the handle **must** cancel any pending timer and ensure that the | ||
| /// associated callback will not fire after the drop returns. |
There was a problem hiding this comment.
"associated callback"? It is unclear what this refers to.
Also who is in charge of cancelling? As I understand it, it is saying that the platform cancels pending timers, yes? If so, the writing style should just be Dropping the handle cancels any pending timer or similar, and it is the platform's job to uphold this contract.
There was a problem hiding this comment.
Re "associated callback", I think (if I understand correctly) this is making a long-range assumption that connects to the signals provider stuff. That should be documented if that's the design you want to go for. Ideally however, we do not use a long-range assumption like that and just literally have a callback registration right here?
| /// A [`TimerHandle`] stub for platforms that have not yet implemented | ||
| /// [`super::TimerProvider`]. All methods panic with `todo!()`. | ||
| pub struct StubTimerHandle(()); |
There was a problem hiding this comment.
Mentioned elsewhere, define this as a pub enum UnsupportedTimerHandle {} instead of supporting its creation.
3 participants
Add a new provider
TimerProviderto create timer for alarm support.For platforms that do not support timer, SIGALRM would only be sent when a syscall returns or a thread is about to be blocked (i.e., in
check_for_interrupt).