--- a/pallets/scheduler-v2/src/lib.rs +++ b/pallets/scheduler-v2/src/lib.rs @@ -96,7 +96,7 @@ BoundedVec, RuntimeDebug, DispatchErrorWithPostInfo, }; use sp_core::H160; -use sp_std::{borrow::Borrow, cmp::Ordering, marker::PhantomData, prelude::*}; +use sp_std::{cmp::Ordering, marker::PhantomData, prelude::*}; pub use weights::WeightInfo; pub use pallet::*; @@ -254,9 +254,9 @@ } impl BlockAgenda { - fn try_push(&mut self, scheduled: ScheduledOf) -> Option { + fn try_push(&mut self, scheduled: ScheduledOf) -> Result> { if self.free_places == 0 { - return None; + return Err(scheduled); } self.free_places = self.free_places.saturating_sub(1); @@ -264,14 +264,14 @@ if (self.agenda.len() as u32) < T::MaxScheduledPerBlock::get() { // will always succeed due to the above check. let _ = self.agenda.try_push(Some(scheduled)); - Some((self.agenda.len() - 1) as u32) + Ok((self.agenda.len() - 1) as u32) } else { match self.agenda.iter().position(|i| i.is_none()) { Some(hole_index) => { self.agenda[hole_index] = Some(scheduled); - Some(hole_index as u32) + Ok(hole_index as u32) } - None => unreachable!("free_places > 0; qed"), + None => unreachable!("free_places was greater than 0; qed"), } } } @@ -473,11 +473,6 @@ }, /// The call for the provided hash was not found so the task has been aborted. CallUnavailable { - task: TaskAddress, - id: Option<[u8; 32]>, - }, - /// The given task was unable to be renewed since the agenda is full at that block. - PeriodicFailed { task: TaskAddress, id: Option<[u8; 32]>, }, @@ -688,12 +683,24 @@ Ok(when) } - fn place_task( + fn mandatory_place_task(when: T::BlockNumber, what: ScheduledOf) { + Self::place_task(when, what, true).expect("mandatory place task always succeeds; qed"); + } + + fn try_place_task( when: T::BlockNumber, what: ScheduledOf, - ) -> Result, (DispatchError, ScheduledOf)> { + ) -> Result, DispatchError> { + Self::place_task(when, what, false) + } + + fn place_task( + mut when: T::BlockNumber, + what: ScheduledOf, + is_mandatory: bool, + ) -> Result, DispatchError> { let maybe_name = what.maybe_id; - let index = Self::push_to_agenda(when, what)?; + let index = Self::push_to_agenda(&mut when, what, is_mandatory)?; let address = (when, index); if let Some(name) = maybe_name { Lookup::::insert(name, address) @@ -706,13 +713,24 @@ } fn push_to_agenda( - when: T::BlockNumber, - what: ScheduledOf, - ) -> Result)> { - let mut agenda = Agenda::::get(when); - let index = agenda - .try_push(what.clone()) - .ok_or((>::AgendaIsExhausted.into(), what))?; + when: &mut T::BlockNumber, + mut what: ScheduledOf, + is_mandatory: bool, + ) -> Result { + let mut agenda; + + let index = loop { + agenda = Agenda::::get(*when); + + match agenda.try_push(what) { + Ok(index) => break index, + Err(returned_what) if is_mandatory => { + what = returned_what; + when.saturating_inc(); + } + Err(_) => return Err(>::AgendaIsExhausted.into()), + } + }; Agenda::::insert(when, agenda); Ok(index) @@ -740,7 +758,7 @@ origin, _phantom: PhantomData, }; - Self::place_task(when, task).map_err(|x| x.0) + Self::try_place_task(when, task) } fn do_cancel( @@ -809,7 +827,7 @@ origin, _phantom: Default::default(), }; - Self::place_task(when, task).map_err(|x| x.0) + Self::try_place_task(when, task) } fn do_cancel_named(origin: Option, id: TaskName) -> DispatchResult { @@ -1075,18 +1093,7 @@ task.maybe_periodic = None; } let wake = now.saturating_add(period); - match Self::place_task(wake, task) { - Ok(_) => {} - Err((_, task)) => { - // TODO: Leave task in storage somewhere for it to be rescheduled - // manually. - T::Preimages::drop(&task.call); - Self::deposit_event(Event::PeriodicFailed { - task: (when, agenda_index), - id: task.maybe_id, - }); - } - } + Self::mandatory_place_task(wake, task); } _ => { if let Some(ref id) = task.maybe_id { --- a/pallets/scheduler-v2/src/tests.rs +++ b/pallets/scheduler-v2/src/tests.rs @@ -319,12 +319,12 @@ .into(), ); // The call is still in the agenda. - assert!(Agenda::::get(4)[0].is_some()); + assert!(Agenda::::get(4).agenda[0].is_some()); }); } #[test] -fn scheduler_handles_periodic_failure() { +fn scheduler_periodic_tasks_always_find_place() { let max_weight: Weight = ::MaximumWeight::get(); let max_per_block = ::MaxScheduledPerBlock::get(); @@ -357,18 +357,17 @@ )); } - // Going to block 24 will emit a `PeriodicFailed` event. run_to_block(24); assert_eq!(logger::log().len(), 6); - assert_eq!( - System::events().last().unwrap().event, - crate::Event::PeriodicFailed { - task: (24, 0), - id: None - } - .into(), - ); + // The periodic task should be postponed + assert_eq!(>::get(29).agenda.len(), 1); + + run_to_block(27); // will call on_initialize(28) + assert_eq!(logger::log().len(), 6); + + run_to_block(28); // will call on_initialize(29) + assert_eq!(logger::log().len(), 7); }); } @@ -596,7 +595,7 @@ )); run_to_block(3); // Scheduled calls are in the agenda. - assert_eq!(Agenda::::get(4).len(), 2); + assert_eq!(Agenda::::get(4).agenda.len(), 2); assert!(logger::log().is_empty()); assert_ok!(Scheduler::cancel_named(RuntimeOrigin::root(), [1u8; 32])); assert_ok!(Scheduler::cancel(RuntimeOrigin::root(), 4, 1)); @@ -669,7 +668,7 @@ )); run_to_block(3); // Scheduled calls are in the agenda. - assert_eq!(Agenda::::get(4).len(), 2); + assert_eq!(Agenda::::get(4).agenda.len(), 2); assert!(logger::log().is_empty()); assert_ok!(Scheduler::cancel_named( system::RawOrigin::Signed(1).into(), @@ -739,7 +738,7 @@ )); run_to_block(3); // Scheduled calls are in the agenda. - assert_eq!(Agenda::::get(4).len(), 2); + assert_eq!(Agenda::::get(4).agenda.len(), 2); assert!(logger::log().is_empty()); assert_noop!( Scheduler::cancel_named(system::RawOrigin::Signed(2).into(), [1u8; 32]),