Skip to content

Commit

Permalink
fix: revert EXC-1838 Run hook after CanisterWasmMemoryLimitExceeded e…
Browse files Browse the repository at this point in the history 
…rror is fixed (#3850)

Reverts #3631
  • Loading branch information
@dragoljub-duric
dragoljub-duric authored Feb 7, 2025
1 parent df5828f commit 9fd33fc
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 121 deletions.
27 changes: 6 additions & 21 deletions rs/execution_environment/src/execution/call_or_task.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub fn execute_call_or_task(
log_dirty_pages: FlagStatus,
deallocation_sender: &DeallocationSender,
) -> ExecuteMessageResult {
let (mut clean_canister, prepaid_execution_cycles, resuming_aborted) =
let (clean_canister, prepaid_execution_cycles, resuming_aborted) =
match prepaid_execution_cycles {
Some(prepaid_execution_cycles) => (clean_canister, prepaid_execution_cycles, true),
None => {
Expand Down Expand Up @@ -151,31 +151,13 @@ pub fn execute_call_or_task(
let helper = match CallOrTaskHelper::new(&clean_canister, &original, deallocation_sender) {
Ok(helper) => helper,
Err(err) => {
if err.code() == ErrorCode::CanisterWasmMemoryLimitExceeded
&& original.call_or_task == CanisterCallOrTask::Task(CanisterTask::OnLowWasmMemory)
{
//`OnLowWasmMemoryHook` is taken from task_queue (i.e. `OnLowWasmMemoryHookStatus` is `Executed`),
// but its was not executed due to error `WasmMemoryLimitExceeded`. To ensure that the hook is executed
// when the error is resolved we need to set `OnLowWasmMemoryHookStatus` to `Ready`. Because of
// the way `OnLowWasmMemoryHookStatus::update` is implemented we first need to remove it from the
// task_queue (which calls `OnLowWasmMemoryHookStatus::update(false)`) followed with `enqueue`
// (which calls `OnLowWasmMemoryHookStatus::update(true)`) to ensure desired behavior.
clean_canister
.system_state
.task_queue
.remove(ic_replicated_state::ExecutionTask::OnLowWasmMemory);
clean_canister
.system_state
.task_queue
.enqueue(ic_replicated_state::ExecutionTask::OnLowWasmMemory);
}
return finish_err(
clean_canister,
original.execution_parameters.instruction_limits.message(),
err,
original,
round,
);
)
}
};

Expand Down Expand Up @@ -371,7 +353,7 @@ impl CallOrTaskHelper {
validate_message(&canister, &original.method)?;

match original.call_or_task {
CanisterCallOrTask::Update(_) | CanisterCallOrTask::Task(_) => {
CanisterCallOrTask::Update(_) => {
let wasm_memory_usage = canister
.execution_state
.as_ref()
Expand All @@ -390,6 +372,9 @@ impl CallOrTaskHelper {
}
}
}
// TODO(RUN-957): Enforce the `wasm_memory_limit` in heartbeat and timer after
// canister logging ships.
CanisterCallOrTask::Task(_) => (),
CanisterCallOrTask::Query(_) => {
if let WasmMethod::CompositeQuery(_) = &original.method {
let user_error = UserError::new(
Expand Down
100 changes: 0 additions & 100 deletions rs/execution_environment/src/execution_environment/tests/canister_task.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1738,103 +1738,3 @@ fn on_low_wasm_memory_is_executed_after_growing_stable_memory() {
NumWasmPages::new(6)
);
}

#[test]
fn on_low_wasm_memory_hook_is_run_after_memory_surpass_limit() {
let mut test = ExecutionTestBuilder::new().with_manual_execution().build();

let update_grow_mem_size = 10;
let hook_grow_mem_size = 5;

let wat: String =
get_wat_with_update_and_hook_mem_grow(update_grow_mem_size, hook_grow_mem_size, true);

let canister_id = test.canister_from_wat(wat.as_str()).unwrap();

// Initially wasm_memory.size = 1
assert_eq!(
test.execution_state(canister_id).wasm_memory.size,
NumWasmPages::new(1)
);

test.ingress_raw(canister_id, "grow_mem", vec![]);

// First ingress messages gets executed.
// wasm_memory.size = 1 + 10 = 11
test.execute_slice(canister_id);

assert_eq!(
test.execution_state(canister_id).wasm_memory.size,
NumWasmPages::new(11)
);

// We update `wasm_memory_limit` to be smaller than `used_wasm_memory`.
test.canister_update_wasm_memory_limit_and_wasm_memory_threshold(
canister_id,
(10 * WASM_PAGE_SIZE_IN_BYTES as u64).into(),
(5 * WASM_PAGE_SIZE_IN_BYTES as u64).into(),
)
.unwrap();

// The update will also trigger `low_wasm_memory` hook.
assert_eq!(
test.state()
.canister_states
.get(&canister_id)
.unwrap()
.system_state
.task_queue
.peek_hook_status(),
OnLowWasmMemoryHookStatus::Ready
);

// Hook execution will not succeed since `used_wasm_memory` > `wasm_memory_limit`.
test.execute_slice(canister_id);

assert_eq!(
test.execution_state(canister_id).wasm_memory.size,
NumWasmPages::new(11)
);

// After execution of the hook fails, hook status will remain `Ready`.
assert_eq!(
test.state()
.canister_states
.get(&canister_id)
.unwrap()
.system_state
.task_queue
.peek_hook_status(),
OnLowWasmMemoryHookStatus::Ready
);

// We fix the error by setting `wasm_memory_limit` > `used_wasm_memory`.
// At the same time:
// `wasm_memory_limit` - `used_wasm_memory` < `wasm_memory_threshold`
// condition for `low_wasm_memory` hook remains satisfied.
// Hence, `low_wasm_memory` hook execution will follow.
test.canister_update_wasm_memory_limit_and_wasm_memory_threshold(
canister_id,
(20 * WASM_PAGE_SIZE_IN_BYTES as u64).into(),
(10 * WASM_PAGE_SIZE_IN_BYTES as u64).into(),
)
.unwrap();

test.execute_slice(canister_id);

assert_eq!(
test.execution_state(canister_id).wasm_memory.size,
NumWasmPages::new(16)
);

assert_eq!(
test.state()
.canister_states
.get(&canister_id)
.unwrap()
.system_state
.task_queue
.peek_hook_status(),
OnLowWasmMemoryHookStatus::Executed
);
}

0 comments on commit 9fd33fc

Please sign in to comment.