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.

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

refactor: [MR-603] Typed canister queues and references #1697

Merged
merged 31 commits into from
Oct 1, 2024

Conversation

alin-at-dfinity
Copy link
Contributor

@alin-at-dfinity alin-at-dfinity commented Sep 26, 2024

MR-603: 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.

…lbacks_with_enqueued_response invariant in CanisterQueues doc comment.
…ry reject response that will result in a critical eror anyway (and the response being dropped).
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.
Copy link
Contributor

@derlerd-dfinity derlerd-dfinity left a comment

Choose a reason for hiding this comment

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

I did a first quick pass to wrap my head around how things are intended to work together. I really like the change!

I left some preliminary comments and will follow up with a more detailed pass.

Base automatically changed from alin/MR-603-shed-inbound-responses to master September 26, 2024 15:39
…d to InboundReference / OutboundReference, to make it clear that the functions may panic (even though the two types are essentially private to the module).
Copy link
Member

@oggy-dfin oggy-dfin left a comment

Choose a reason for hiding this comment

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

Thanks for making this more robust. I did a first pass over the "meat", though not the tests yet. I wonder:

  • can we make the context the property of the T type in Reference<T>. That would seem to get rid of a bunch of asserts that we currently have
  • do we even need Reference::context to be visible from outside of message_pool? I.e., outside of TryFrom<QueueItem>?

…e item types declare their own context (inbound vs outbound) and use that when constructing a new reference of the given type.
@alin-at-dfinity alin-at-dfinity added this pull request to the merge queue Oct 1, 2024
Merged via the queue into master with commit 3221c59 Oct 1, 2024
24 checks passed
@alin-at-dfinity alin-at-dfinity deleted the alin/MR-603-typed-queues branch October 1, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants