feat: [MR-552] Implement callback expiration #1699
Conversation
…eadlineExpired and ResponseDropped.
…ed-inbound-responses
…ed-inbound-responses
…lbacks_with_enqueued_response invariant in CanisterQueues doc comment.
…ed-inbound-responses
…ry reject response that will result in a critical eror anyway (and the response being dropped).
… as a dummy canister ID.
Assign type parameters to canister queues and references, to designate them as either input/inbound or output/outbound. Ensures that input queues can only hold inbound references; and output queues can only hold outbound references. Implements separate logic (for inbound and outbound references) for determining staleness, lookup and removal.
alin-at-dfinity
requested review from
stiegerc,
derlerd-dfinity and
oggy-dfin
… the two specific `MessageStore<_>` implementations into `MessageStoreImpl`.
…d to InboundReference / OutboundReference, to make it clear that the functions may panic (even though the two types are essentially private to the module).
…e item types declare their own context (inbound vs outbound) and use that when constructing a new reference of the given type.
Generate compact reject responses upon best-effort callback expiration. And inflate them into SYS_UNKNOWN reject responses when peeking/popping.
alin-at-dfinity
force-pushed
the
alin/MR-552-callback-expiration
branch
from
32c0d32
to
41d6b69
Compare
| fn get(&self, reference: InboundReference) -> CanisterInput { | ||
| assert_eq!(Context::Inbound, reference.context()); | ||
|
|
||
| if let Some(msg) = self.pool.get(reference) { | ||
| debug_assert!(!self.expired_callbacks.contains_key(&reference)); | ||
| debug_assert!(!self.shed_responses.contains_key(&reference)); | ||
| return msg.clone().into(); | ||
| } else if reference.class() == Class::BestEffort && reference.kind() == Kind::Response { | ||
| if let Some(callback_id) = self.expired_callbacks.get(&reference) { | ||
| debug_assert!(!self.shed_responses.contains_key(&reference)); | ||
| return CanisterInput::DeadlineExpired(*callback_id); | ||
| } else if let Some(callback_id) = self.shed_responses.get(&reference) { | ||
| return CanisterInput::ResponseDropped(*callback_id); | ||
| } | ||
| } | ||
|
|
||
| panic!("stale reference at the front of input queue"); | ||
| } |
There was a problem hiding this comment.
This seems a bit odd. get() and take() is almost the same function. For one you could probably refactor that using a helper function to remove the duplication. Why is get() returning a clone though? It's usually Option<&T>.
There was a problem hiding this comment.
I suppose it's possible to either pass a function pointer or use generics, but the two functions aren't all that large. And trying to extract the common structure might simply add complexity and make it less readable. If you don't have a strong objection, I'd rather leave it as is.
MessageStore<CanisterInput>::get() returns a clone rather than a reference because we have to build a CanisterInput enum on the fly and Rust (with good reason) won't let you return a reference to a temporary value. MessageStore<RequestOrResponse>::get() OTOH, will return an Option<&RequestOrResponse> because it holds RequestOrResponse values within, so it can just return references to them.
There was a problem hiding this comment.
Fair enough. How about a function
fn is_best_effort_response(&self) -> bool {
self.class() == Class::BestEffort && self.kind() == Kind::Response
}or something like that? Maybe also something similar for expired_callbacks and shed_responses since both of these expressions also appear in the complicated logical expression @derlerd-dfinity mentioned below?
There was a problem hiding this comment.
That's a good point, actually. At some point I was thinking of adding fancier methods to Id / Reference, so that you could say BestEffort && Request by applying a single mask and a single binary operation (as opposed to filtering for one bit and then separately for the other). But now that you brought this up, I took a quick look and the two combinations we care about are "inbound best-effort response" (because these can be shed and result in an edge case); and "outbound guaranteed-response requests" (because these are the only non-best-effort messages that expire). So I added explicit methods for the two cases (which are just id & BITMASK == Inbound | BestEffort | Response, which compiles to id & x = y).
rs/replicated_state/src/canister_state/queues/message_pool/tests.rs
Outdated
Show resolved
Hide resolved
rs/replicated_state/src/canister_state/queues/message_pool/tests.rs
Outdated
Show resolved
Hide resolved
…eStore::callbacks_with_enqueued_response(); fix typo; move inner function definition before use.
…::is_stale() for better readability. Make all asserts for the right context type into debug_asserts().
| @@ -74,6 +74,20 @@ impl CanisterQueuesFixture { | |||
| ) | |||
| } | |||
|
|
|||
| fn try_push_deadline_expired_input(&mut self) -> Result<(), String> { | |||
| self.last_callback_id += 1; | |||
There was a problem hiding this comment.
Absolutely not for this PR, but conceptually there should be one callback ID for every successful push_output_request. If you're going in the direction of bundling the CallContextManager together with the queues, I do wonder if we have any practical chance of actually enforcing this, i.e., somehow have CallbackId be only generated through push_output_request. I realize you'd need to thread some canister-global state through to keep the numbers unique across the different queues. And I now also realize that there are two next_callback_id fields, one in SandboxSafeSystemState, and one in CallContextManager, and I have no idea what's going on there.
There was a problem hiding this comment.
I also hope it can be done. My eventual aim is to turn SystemState (at least the parts of it that deal with CanisterQueues plus CallContextManager) into a state machine (in-between message executions; executing a request; executing a response; etc.). It should be possible to automatically create callbacks as requests are being enqueued, although it may require refactoring on the Execution end.
The reason why both CallContextManager and SandboxSafeSystemState have a next_callback_id is that the latter is trying to guess the callback IDs that the former will assign the new callbacks when they are registered (likely in order to populate them in the Request). So bundling callback creation and request enqueuing should also get rid of this weirdness.
| @@ -276,7 +276,7 @@ impl CallContextManagerStats { | |||
|
|
|||
| /// Calculates the stats for the given call contexts and callbacks. | |||
| /// | |||
| /// Time complexity: `O(|call_contexts| + |callbacks|)`. | |||
| /// Time complexity: `O(n)`. | |||
There was a problem hiding this comment.
Nit, but I found the previous formulation more informative, it's not really clear to me what n is if I just see that.
There was a problem hiding this comment.
As explained elsewhere to Christian, my goal with these notes is to give the caller an idea of whether it's OK to use this in a tight loop or not (and ideally not, since anything that isn't constant or logarithmic time has no business being called from the scheduler or Message Routing). If you check, virtually all the functions with this kind of note are invariant checks or "calculate X" sort of things, that should only be called from debug_asserts or while loading the state.
What the exact n is, is irrelevant, since it's more or less under the control of users or an attacker. Saying e.g. O(|shed_responses|) may lead someone to conclude "Oh, this is likely to be a small number, so it's fine to use it for X".
…_response() and Reference::is_outbound_guaranteed_request() methods and use them to more concisely check for the respective message types.
MR-552: Generate compact reject responses upon best-effort callback expiration. And inflate them into SYS_UNKNOWN reject responses when peeking/popping.