Add opt-in handlers for before sleeping and before handling events #144
Conversation
src/loop_logic.rs
Outdated
| pub(crate) sources: RefCell<Slab<Rc<dyn EventDispatcher<Data> + 'l>>>, | ||
| pub(crate) sources_with_additional_lifetime_events: AdditionalLifetimeEventsSet, |
There was a problem hiding this comment.
Could we just duplicate the way sources are defined instead of rolling new abstraction? And create an iterator which simply chains both when needed?
DJMcNab
force-pushed
the
additional-lifecycle
branch
from
55165b5
to
9bfaa1c
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #144 +/- ##
==========================================
- Coverage 88.79% 87.11% -1.68%
==========================================
Files 14 14
Lines 1669 1739 +70
==========================================
+ Hits 1482 1515 +33
- Misses 187 224 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
src/sources/mod.rs
Outdated
| /// Use this to perform operations which must be done before polling, | ||
| /// but which may conflict with other event handlers. For example, | ||
| /// if polling requires a lock to be taken. | ||
| fn before_will_sleep(&mut self) -> crate::Result<Option<(Readiness, Token)>> { |
There was a problem hiding this comment.
What about naming this before_sleep instead? This matches better the naming pattern of the other method.
|
Also, it looks like the |
DJMcNab
force-pushed
the
additional-lifecycle
branch
from
dce9914
to
831ddaa
Compare
There was a problem hiding this comment.
Can't we at least do something like that? it should at least make it more readable. The main pain point is still Rc stuff. Also, I'm not sure what this all logic wrt Registration token, TokenFactory tries to do, because it's not consistent with what it does. Like it seems that sometimes sub sources are being passed, sometimes real sources, it's just all over the place, but I don't want to go into details with it.
My patch is likely broken with regards to those tokens, but generally it's just more consistent with what already is. I don't disagree with the idea to cache tokens to improve lookups, it' just I'd rather follow the current abstractions.
diff --git a/src/io.rs b/src/io.rs
index ffbb857..980cd59 100644
--- a/src/io.rs
+++ b/src/io.rs
@@ -18,7 +18,7 @@ use rustix::fs::{fcntl_getfl, fcntl_setfl, OFlags};
#[cfg(feature = "futures-io")]
use futures_io::{AsyncRead, AsyncWrite, IoSlice, IoSliceMut};
-use crate::AdditionalLifetimeEventsRegister;
+use crate::AdditionalLifecycleEventsSet;
use crate::{
loop_logic::{LoopInner, MAX_SOURCES_MASK},
sources::EventDispatcher,
@@ -241,7 +241,7 @@ impl<Data> EventDispatcher<Data> for RefCell<IoDispatcher> {
fn register(
&self,
_: &mut Poll,
- _: AdditionalLifetimeEventsRegister<'_>,
+ _: &mut AdditionalLifecycleEventsSet,
_: &mut TokenFactory,
) -> crate::Result<()> {
// registration is handled by IoLoopInner
@@ -251,7 +251,7 @@ impl<Data> EventDispatcher<Data> for RefCell<IoDispatcher> {
fn reregister(
&self,
_: &mut Poll,
- _: AdditionalLifetimeEventsRegister<'_>,
+ _: &mut AdditionalLifecycleEventsSet,
_: &mut TokenFactory,
) -> crate::Result<bool> {
// registration is handled by IoLoopInner
@@ -261,7 +261,8 @@ impl<Data> EventDispatcher<Data> for RefCell<IoDispatcher> {
fn unregister(
&self,
poll: &mut Poll,
- _: AdditionalLifetimeEventsRegister<'_>,
+ _: &mut AdditionalLifecycleEventsSet,
+ _: TokenFactory,
) -> crate::Result<bool> {
let disp = self.borrow();
if disp.is_registered {
diff --git a/src/loop_logic.rs b/src/loop_logic.rs
index a0ab8e5..661e58d 100644
--- a/src/loop_logic.rs
+++ b/src/loop_logic.rs
@@ -15,7 +15,7 @@ use slab::Slab;
use crate::sources::{Dispatcher, EventSource, Idle, IdleDispatcher};
use crate::sys::{Notifier, PollEvent};
use crate::{
- AdditionalLifetimeEventsSet, EventDispatcher, InsertError, Poll, PostAction, TokenFactory,
+ AdditionalLifecycleEventsSet, EventDispatcher, InsertError, Poll, PostAction, TokenFactory,
};
type IdleCallback<'i, Data> = Rc<RefCell<dyn IdleDispatcher<Data> + 'i>>;
@@ -58,7 +58,7 @@ pub struct RegistrationToken {
pub(crate) struct LoopInner<'l, Data> {
pub(crate) poll: RefCell<Poll>,
pub(crate) sources: RefCell<Slab<Rc<dyn EventDispatcher<Data> + 'l>>>,
- pub(crate) sources_with_additional_lifecycle_events: AdditionalLifetimeEventsSet,
+ pub(crate) sources_with_additional_lifecycle_events: RefCell<AdditionalLifecycleEventsSet>,
idles: RefCell<Vec<IdleCallback<'l, Data>>>,
pending_action: Cell<PostAction>,
}
@@ -140,9 +140,10 @@ impl<'l, Data> LoopHandle<'l, Data> {
let key = sources.insert(dispatcher.clone_as_event_dispatcher());
let ret = sources.get(key).unwrap().register(
&mut poll,
- self.inner
+ &mut self
+ .inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(RegistrationToken { key }),
+ .borrow_mut(),
&mut TokenFactory::new(key),
);
@@ -178,9 +179,10 @@ impl<'l, Data> LoopHandle<'l, Data> {
if let Some(source) = self.inner.sources.borrow().get(token.key) {
source.register(
&mut self.inner.poll.borrow_mut(),
- self.inner
+ &mut self
+ .inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(*token),
+ .borrow_mut(),
&mut TokenFactory::new(token.key),
)?;
}
@@ -195,9 +197,10 @@ impl<'l, Data> LoopHandle<'l, Data> {
if let Some(source) = self.inner.sources.borrow().get(token.key) {
if !source.reregister(
&mut self.inner.poll.borrow_mut(),
- self.inner
+ &mut self
+ .inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(*token),
+ .borrow_mut(),
&mut TokenFactory::new(token.key),
)? {
// we are in a callback, store for later processing
@@ -214,9 +217,11 @@ impl<'l, Data> LoopHandle<'l, Data> {
if let Some(source) = self.inner.sources.borrow().get(token.key) {
if !source.unregister(
&mut self.inner.poll.borrow_mut(),
- self.inner
+ &mut self
+ .inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(*token),
+ .borrow_mut(),
+ TokenFactory::new(token.key),
)? {
// we are in a callback, store for later processing
self.inner.pending_action.set(PostAction::Disable);
@@ -230,9 +235,11 @@ impl<'l, Data> LoopHandle<'l, Data> {
if let Some(source) = self.inner.sources.borrow_mut().try_remove(token.key) {
if let Err(e) = source.unregister(
&mut self.inner.poll.borrow_mut(),
- self.inner
+ &mut self
+ .inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(token),
+ .borrow_mut(),
+ TokenFactory::new(token.key),
) {
log::warn!(
"[calloop] Failed to unregister source from the polling system: {:?}",
@@ -320,16 +327,17 @@ impl<'l, Data> EventLoop<'l, Data> {
) -> crate::Result<()> {
let now = Instant::now();
{
- let mut extra_lifecycle_sources = self
+ let extra_lifecycle_sources = &mut self
.handle
.inner
.sources_with_additional_lifecycle_events
- .values
- .borrow_mut();
+ .borrow_mut()
+ .values;
+
let sources = &self.handle.inner.sources.borrow();
- for (source, has_event) in &mut *extra_lifecycle_sources {
+ for (source_key, has_event) in &mut *extra_lifecycle_sources {
*has_event = false;
- if let Some(disp) = sources.get(source.key) {
+ if let Some(disp) = sources.get(*source_key) {
if let Some((readiness, token)) = disp.before_sleep()? {
// Wake up instantly after polling if we recieved an event
timeout = Some(Duration::ZERO);
@@ -363,21 +371,21 @@ impl<'l, Data> EventLoop<'l, Data> {
}
};
{
- let mut extra_lifecycle_sources = self
+ let extra_lifecycle_sources = &mut self
.handle
.inner
.sources_with_additional_lifecycle_events
- .values
- .borrow_mut();
+ .borrow_mut()
+ .values;
if !extra_lifecycle_sources.is_empty() {
- for (source, has_event) in &mut *extra_lifecycle_sources {
+ for (source_key, has_event) in &mut *extra_lifecycle_sources {
for event in &events {
- *has_event |= (event.token.key & MAX_SOURCES_MASK) == source.key;
+ *has_event |= (event.token.key & MAX_SOURCES_MASK) == *source_key;
}
}
}
- for (source, has_event) in &*extra_lifecycle_sources {
- if let Some(disp) = self.handle.inner.sources.borrow().get(source.key) {
+ for (source_key, has_event) in &*extra_lifecycle_sources {
+ if let Some(disp) = self.handle.inner.sources.borrow().get(*source_key) {
disp.before_handle_events(*has_event);
} else {
unreachable!()
@@ -414,24 +422,23 @@ impl<'l, Data> EventLoop<'l, Data> {
PostAction::Reregister => {
disp.reregister(
&mut self.handle.inner.poll.borrow_mut(),
- self.handle
+ &mut self
+ .handle
.inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(RegistrationToken {
- key: registroken_token,
- }),
+ .borrow_mut(),
&mut TokenFactory::new(event.token.key),
)?;
}
PostAction::Disable => {
disp.unregister(
&mut self.handle.inner.poll.borrow_mut(),
- self.handle
+ &mut self
+ .handle
.inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(RegistrationToken {
- key: registroken_token,
- }),
+ .borrow_mut(),
+ TokenFactory::new(event.token.key),
)?;
}
PostAction::Remove => {
@@ -456,12 +463,12 @@ impl<'l, Data> EventLoop<'l, Data> {
let mut poll = self.handle.inner.poll.borrow_mut();
if let Err(e) = disp.unregister(
&mut poll,
- self.handle
+ &mut self
+ .handle
.inner
.sources_with_additional_lifecycle_events
- .create_register_for_token(RegistrationToken {
- key: registroken_token,
- }),
+ .borrow_mut(),
+ TokenFactory::new(registroken_token),
) {
log::warn!(
"[calloop] Failed to unregister source from the polling system: {:?}",
diff --git a/src/sources/mod.rs b/src/sources/mod.rs
index 4ccc1d5..3111dfa 100644
--- a/src/sources/mod.rs
+++ b/src/sources/mod.rs
@@ -4,7 +4,7 @@ use std::{
rc::Rc,
};
-use crate::{sys::TokenFactory, Poll, Readiness, RegistrationToken, Token};
+use crate::{sys::TokenFactory, Poll, Readiness, Token};
pub mod channel;
#[cfg(feature = "executor")]
@@ -314,13 +314,13 @@ where
fn register(
&self,
poll: &mut Poll,
- mut additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
token_factory: &mut TokenFactory,
) -> crate::Result<()> {
let mut this = self.borrow_mut();
if this.needs_additional_lifecycle_events {
- additional_lifecycle_register.register();
+ additional_lifecycle_register.register(token_factory.key);
}
this.source.register(poll, token_factory)
}
@@ -328,13 +328,13 @@ where
fn reregister(
&self,
poll: &mut Poll,
- mut additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
token_factory: &mut TokenFactory,
) -> crate::Result<bool> {
if let Ok(mut me) = self.try_borrow_mut() {
me.source.reregister(poll, token_factory)?;
if me.needs_additional_lifecycle_events {
- additional_lifecycle_register.register();
+ additional_lifecycle_register.register(token_factory.key);
}
Ok(true)
} else {
@@ -345,12 +345,13 @@ where
fn unregister(
&self,
poll: &mut Poll,
- mut additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
+ token_factory: TokenFactory,
) -> crate::Result<bool> {
if let Ok(mut me) = self.try_borrow_mut() {
me.source.unregister(poll)?;
if me.needs_additional_lifecycle_events {
- additional_lifecycle_register.unregister();
+ additional_lifecycle_register.unregister(token_factory.key);
}
Ok(true)
} else {
@@ -382,60 +383,43 @@ pub(crate) trait EventDispatcher<Data> {
fn register(
&self,
poll: &mut Poll,
- additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
token_factory: &mut TokenFactory,
) -> crate::Result<()>;
fn reregister(
&self,
poll: &mut Poll,
- additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
token_factory: &mut TokenFactory,
) -> crate::Result<bool>;
fn unregister(
&self,
poll: &mut Poll,
- additional_lifecycle_register: AdditionalLifetimeEventsRegister<'_>,
+ additional_lifecycle_register: &mut AdditionalLifecycleEventsSet,
+ token_factory: TokenFactory,
) -> crate::Result<bool>;
fn before_sleep(&self) -> crate::Result<Option<(Readiness, Token)>>;
fn before_handle_events(&self, was_awoken: bool);
}
+/// Track sources which require extra lifecycle events.
#[derive(Default)]
-/// The list of events
-pub(crate) struct AdditionalLifetimeEventsSet {
+pub(crate) struct AdditionalLifecycleEventsSet {
/// The list of events. The boolean indicates whether the source had an event
/// - this is stored in this list because we need to know this value for each
/// item at once - that is, to avoid allocating in the hot loop
- pub(crate) values: RefCell<Vec<(RegistrationToken, bool)>>,
+ pub(crate) values: Vec<(usize, bool)>,
}
-
-impl AdditionalLifetimeEventsSet {
- pub(crate) fn create_register_for_token(
- &self,
- token: RegistrationToken,
- ) -> AdditionalLifetimeEventsRegister {
- AdditionalLifetimeEventsRegister { set: self, token }
- }
-}
-
-/// To ensure that registering the events which need additional lifecycle events
-/// happens correctly, we do this within the implementations of `EventDispatcher`
-/// `AdditionalLifetimeEventsRegister` is a helper type to centralise this logic
-pub(crate) struct AdditionalLifetimeEventsRegister<'a> {
- set: &'a AdditionalLifetimeEventsSet,
- token: RegistrationToken,
-}
-
-impl<'a> AdditionalLifetimeEventsRegister<'a> {
- fn register(&mut self) {
- self.set.values.borrow_mut().push((self.token, false))
+impl AdditionalLifecycleEventsSet {
+ fn register(&mut self, token: usize) {
+ self.values.push((token, false))
}
- fn unregister(&mut self) {
- self.set.values.borrow_mut().retain(|it| it.0 != self.token)
+ fn unregister(&mut self, token: usize) {
+ self.values.retain(|it| it.0 != token)
}
}
|
So there are three changes you're asking for there:
|
|
Just to make sure I didn't miss anything, point 1) is just about having the Regarding 2/3, I think I like the way it is currently on the PR, keeping the type-safety and using the token from he available tokenfactory is good imo, especially given this concerns crate-internals anyway. |
|
Also, |
CHANGELOG.md
Outdated
| - Replace the `nix` crate with standard library I/O errors and the `rustix` | ||
| crate. | ||
| crate.s |
There was a problem hiding this comment.
Looks like a typo snuck in there? Also actually I don't get why "crate" is on its own line there, let's just put it at the end of the previous line.
Cargo.toml
Outdated
| @@ -24,7 +24,7 @@ bitflags = "1.2" | |||
| futures-io = { version = "0.3.5", optional = true } | |||
| io-lifetimes = "1.0.3" | |||
| log = "0.4" | |||
| nix = { version = "0.26", default-features = false, features = ["signal"], optional = true } | |||
| nix = { version = "0.26", default-features = false, features = ["signal",], optional = true } | |||
There was a problem hiding this comment.
What's the point of this change?
There was a problem hiding this comment.
I have an extension which formats TOML files. In 0364c1c, I reverted some edits made by this manually, but missed this comma
src/loop_logic.rs
Outdated
| if !extra_lifecycle_sources.values.is_empty() { | ||
| for (source, has_event) in &mut *extra_lifecycle_sources.values { | ||
| for event in &events { | ||
| *has_event |= (event.token.key & MAX_SOURCES_MASK) == source.key; | ||
| } | ||
| } | ||
| } | ||
| for (source, has_event) in &*extra_lifecycle_sources.values { | ||
| if let Some(disp) = self.handle.inner.sources.borrow().get(source.key) { | ||
| disp.before_handle_events(*has_event); | ||
| } else { | ||
| unreachable!() | ||
| } | ||
| } |
There was a problem hiding this comment.
Couldn't these two loops be fused into a single one?
And then, it looks like has_event is only used by those two loops, so it can probably be removed altogether from extra_lifecycle_sources, no?
There was a problem hiding this comment.
I don't believe so - we need to run all before_handle_events handlers, even those which didn't receive events
I suppose we could have two different handler methods for the different parts, so that we don't need to track was_awoken. That means that if a source needs was_awoken, it would have to track it itself. But that's fine
I'm fairly sure we do need two loops though
There was a problem hiding this comment.
But what about something like this?
for each extra_lifecycle_source {
let has_event = /* loop over events to check if that source was woken up */;
disp.before_handle_events(has_event);
}
src/sources/mod.rs
Outdated
| /// If this returns Ok(Some), polling will shortcircuit, and | ||
| /// your event handler will be called with the returned Token and Readiness |
There was a problem hiding this comment.
The current implementation does still invoke poll with a timeout of 0 even if the before_sleep() method returned Ok(Some(...)), which means it is possible that poll generates another legit event. We need to document what happens in this case.
What the current implementation does is:
before_handle_events()is invoked if a real event was generated, independently of whatbefore_sleep()returned- If a real event is generated in addition to the one from
before_sleep(), thendispatch_events()is invoked twice.
Imo this is the correct thing to do (to handle composite sources with multiple tokens), but this needs to be documented clearly.
src/sources/mod.rs
Outdated
| /// code may run. This could be used to drop a lock obtained in | ||
| /// [`EventSource::before_sleep`] | ||
| #[allow(unused_variables)] | ||
| fn before_handle_events(&mut self, was_awoken: bool) {} |
There was a problem hiding this comment.
I'm wondering if it might be relevant to pass the (Token, Readiness) couple as argument to this, instead of just a was_awoken bool. Again as a way to properly handle composite sources that may have multiple underlying FDs and tokens.
There was a problem hiding this comment.
To be more precise, I see two ways to achieve this:
- make the function take as argument
Option<(Token, Readiness)>, and say that it is called once withNoneif no event is received, or once ore more times withSome()if events were triggered - make the function take a
&dyn Iterator<Item=(Token, Readiness)>>as argument, with that iterator being empty if no event was triggered
src/sources/mod.rs
Outdated
| /// | ||
| /// If this returns Ok(Some), this will be treated as an event arriving in polling, and | ||
| /// your event handler will be called with the returned `Token` and `Readiness`. | ||
| /// Polling will however still occur, so additional events from this or other sources |
There was a problem hiding this comment.
"Polling will however still occur, but with a timeout of 0
src/sources/mod.rs
Outdated
| /// Please note, the iterator will also include any synthetic event returned from | ||
| /// [`EventSource::before_sleep`] |
There was a problem hiding this comment.
Do we actually want that?
The source can already internally track if it gave a synthetic event or not, and at least for the wayland socket, it'd be more useful in this method to know if other real events occurred during the polling.
In both cases the wayland source will be able to work with it, but it seems to me including the synthetic events in this iterator only increases the complexity of implementing the sources.
|
Alright, thanks a lot this is perfect! 👍 |
Solves #127. Designed to work with Smithay/wayland-rs#650 and to be used to fix #glazier > Freeze with Wayland egl