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

forEachPublicationRecord is not sufficiently virtual #7302

Open
mhofman opened this issue Apr 1, 2023 · 2 comments
Open

forEachPublicationRecord is not sufficiently virtual #7302

mhofman opened this issue Apr 1, 2023 · 2 comments
Labels
bug Something isn't working notifier vaults_triage DO NOT USE

Comments

@mhofman
Copy link
Member

mhofman commented Apr 1, 2023

Describe the bug

pipeTopicToStorage was made as a replacement for makeStoredPublishKit that is compatible with durable publish kits but it kept using forEachPublicationRecord, which keeps all objects in heap: the storageNode (through consumeValue), the subscriber and its iterator.

Instead forEachPublicationRecord should use the watchPromise pattern, using a durable or virtual promise handler as appropriate for the use case.

Expected behavior

No heap usage related to the usage of pipeTopicToStorage or makeStoredPublishKit.

No use of forEachPublicationRecord (explicitly or transitively) in durable contracts.

Additional context

Found while investigating #7297

@mhofman mhofman added bug Something isn't working notifier labels Apr 1, 2023
@ivanlei ivanlei added vaults_triage DO NOT USE and removed vaults_triage DO NOT USE labels Apr 3, 2023
@turadg
Copy link
Member

turadg commented Apr 3, 2023

Note that this limitation of pipeTopicToStorage was understood at the time it was created. #6893 was queued up to solve it.

Now that we have #7303 I propose that we simply remove pipeTopicToStorage. That can be part of #7300

We still need to decide about other uses of forEachPublicationRecord. I propose that this issue be scoped to that.

@ivanlei ivanlei added the vaults_triage DO NOT USE label Apr 3, 2023
@turadg
Copy link
Member

turadg commented Apr 3, 2023

Given that we have #7300 I've rescoped this to fixing/removing forEachPublicationRecord.

@turadg turadg changed the title pipeTopicToStorage is not sufficiently virtual forEachPublicationRecord is not sufficiently virtual Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working notifier vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

3 participants