Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for Issue 4762 #4803

Merged
merged 12 commits into from
Oct 16, 2024
Merged

Fix for Issue 4762 #4803

Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8973908
Fix issue 4762
Jun 14, 2024
c39556e
Ready for PR
Jun 15, 2024
05197d1
Update inline docs
Jun 16, 2024
43add5d
Updates per code review
Jun 26, 2024
d3e03ff
Add more notes per review
Jun 30, 2024
71d8171
Merge branch 'master' into issue-4762
bkchr Jul 1, 2024
8264ab8
Add prdoc
Jul 4, 2024
1a04152
Merge branch 'master' into issue-4762
bkchr Sep 22, 2024
6bbf1a3
Apply suggestions from code review
bkchr Sep 22, 2024
b3551d6
Fix issues
bkchr Oct 16, 2024
2927182
Merge branch 'master' into issue-4762
bkchr Oct 16, 2024
eaba016
Update prdoc/pr_4803.prdoc
bkchr Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions substrate/frame/message-queue/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,17 +645,19 @@ pub mod pallet {
#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
let context = ServiceQueuesContext::OnInitialize;
gotnoshoeson marked this conversation as resolved.
Show resolved Hide resolved
if let Some(weight_limit) = T::ServiceWeight::get() {
Self::service_queues(weight_limit)
Self::service_queues(weight_limit, context)
gotnoshoeson marked this conversation as resolved.
Show resolved Hide resolved
} else {
Weight::zero()
}
}

fn on_idle(_n: BlockNumberFor<T>, remaining_weight: Weight) -> Weight {
let context = ServiceQueuesContext::OnIdle;
gotnoshoeson marked this conversation as resolved.
Show resolved Hide resolved
if let Some(weight_limit) = T::IdleMaxServiceWeight::get() {
// Make use of the remaining weight to process enqueued messages.
Self::service_queues(weight_limit.min(remaining_weight))
Self::service_queues(weight_limit.min(remaining_weight), context)
gotnoshoeson marked this conversation as resolved.
Show resolved Hide resolved
} else {
Weight::zero()
}
Expand Down Expand Up @@ -774,6 +776,16 @@ enum MessageExecutionStatus {
StackLimitReached,
}

/// The context to pass to service_queues through on_idle and on_initialize hooks
/// We don't want to throw the defensive message if called from on_idle hook
#[derive(PartialEq)]
enum ServiceQueuesContext {
/// Context of on_idle hook
OnIdle,
/// Context of on_initialize hook
OnInitialize,
}

impl<T: Config> Pallet<T> {
/// Knit `origin` into the ready ring right at the end.
///
Expand Down Expand Up @@ -1554,13 +1566,20 @@ impl<T: Get<O>, O: Into<u32>> Get<u32> for IntoU32<T, O> {
impl<T: Config> ServiceQueues for Pallet<T> {
type OverweightMessageAddress = (MessageOriginOf<T>, PageIndex, T::Size);

fn service_queues(weight_limit: Weight) -> Weight {
fn service_queues(weight_limit: Weight, context: ServiceQueuesContext) -> Weight {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that this is implementing a trait function. Can you maybe put the inner logic into a new function and call that from here?

Having the ServiceQueuesContext in the interface would be ugly otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pushed a new commit last night (PST). Now that I'm re-reading this I might not have implemented the way you requested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, the code does not compile? You are implementing a trait function. Please try to run the pallet tests locally.

let mut weight = WeightMeter::with_limit(weight_limit);

// Get the maximum weight that processing a single message may take:
let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| {
defensive!("Not enough weight to service a single message.");
Weight::zero()
// throw defensive message when service_queues is called from on_initialize
// don't throw message when service_queues is called from on_idle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the reason here, that it doesn't matter if on_idle doesn't have enough weight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense. Thank you

match context {
ServiceQueuesContext::OnInitialize => {
defensive!("Not enough weight to service a single message.");
Weight::zero()
},
ServiceQueuesContext::OnIdle => Weight::zero(),
}
gotnoshoeson marked this conversation as resolved.
Show resolved Hide resolved
});

match with_service_mutex(|| {
Expand Down
Loading