fix: [EXC-1831] Low Wasm memory hook will run once the canister's unfrozen if it was scheduled before freezing #3455
Conversation
| //`OnLowWasmMemoryHook` is taken from task_queue (i.e. `OnLowWasmMemoryHookStatus` is `Executed`), | ||
| // but its was not executed due to the freezing of the canister. To ensure that the hook is executed | ||
| // when the canister is unfrozen 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. | ||
| canister | ||
| .system_state | ||
| .task_queue | ||
| .remove(ic_replicated_state::ExecutionTask::OnLowWasmMemory); | ||
| canister | ||
| .system_state | ||
| .task_queue | ||
| .enqueue(ic_replicated_state::ExecutionTask::OnLowWasmMemory); |
There was a problem hiding this comment.
This logic may also be implemented directly on the TaskQueue, but it will require an additional method (let's say reenable_on_low_wasm_memory_hook), which will further diverge from the queue interface. So I opted for implementing it this way, but I do not have a strong opinion on this.
There was a problem hiding this comment.
Sorry, I might miss the context. Wouldn't it be better if we just follow the GlobalTimer pattern, i.e. set/reset the 'low memory' hook in the same places?
There was a problem hiding this comment.
I do not think these two are related (though I do not have the whole context about the implementation of GlobalTimer), here I think we have an easier problem. We just have a task that is taken from the task_queue, and if it is not executed because of the freezing of the canister, we want it to be executed as soon as it is unfrozen. Because of the way hook status is implemented to be StateMachine to make status to be Ready, given that the current status is Executed we first need to make it NotSatisfied and then Ready.
|
An alternative approach was, that instead of always updating the hook status to |
dragoljub-duric
changed the title
Problem:
@mraszyk noticed here that property:
”If the canister gets frozen immediately after the function is scheduled for execution, the function will run once the canister's unfrozen if the canister's wasm memory remains above the threshold t.”
is missing.
Solution:
When execution of the hook is stopped because of the freezing canister, add the hook again to the task queue.