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

collection.clear(pattern) corrupts entryCount by setting it to zero #10007

Closed
warner opened this issue Aug 30, 2024 · 0 comments · Fixed by #10008
Closed

collection.clear(pattern) corrupts entryCount by setting it to zero #10007

warner opened this issue Aug 30, 2024 · 0 comments · Fixed by #10008
Assignees
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Aug 30, 2024

Describe the bug

@mhofman noticed that the clearInternal() function for (strong) virtual/durable collections always sets the |entryCount metadata field to 0, even if it was called with a pattern that prevents some keys from being deleted.

syscall.vatstoreSet(prefix('|entryCount'), '0');

To Reproduce

const c = makeScalarBigMapStore();
c.init('a', 'value-a');
c.init('b', 'value-b');
c.clear(M.eq('a');

assert(c.has('a') === false); // ok
assert(c.has('b') === true); // ok
assert(c.getSize() === 1); // broken, returns 0

To Fix

We need to change clearInternal() to only set the count to zero in the clearInternalFull() branch, and decrement it by the number of deleted keys in the other branch.

The fix must also consider isDeleting, which is true when we're deleting the whole collection, which means we don't need to update the count (because we're going to delete the metadata too, once this function returns).

@warner warner added bug Something isn't working SwingSet package: SwingSet liveslots requires vat-upgrade to deploy changes labels Aug 30, 2024
warner added a commit that referenced this issue Aug 31, 2024
The `.clear()` method, on (strong) collections, will delete all entries.
But it can also be called with `keyPatt` and/or `valuePatt` argument,
and only the matching entries will be deleted.

Previously, the `|entryCount` metadata field was unconditionally set
to zero for any call to `.clear()`, even those with patterns which
caused some entries to be retained. This caused a subsequent
`collection.getSize()` to return the wrong value, yielding zero even
if the collection still had entries.

This commit fixes the logic to decrement the `|entryCount` field by
the number of entries actually deleted. As an optimization, it is
still set directly to zero if `clear()` is not limited by patterns.

fixes #10007
warner added a commit that referenced this issue Aug 31, 2024
The `.clear()` method, on (strong) collections, will delete all entries.
But it can also be called with `keyPatt` and/or `valuePatt` argument,
and only the matching entries will be deleted.

Previously, the `|entryCount` metadata field was unconditionally set
to zero for any call to `.clear()`, even those with patterns which
caused some entries to be retained. This caused a subsequent
`collection.getSize()` to return the wrong value, yielding zero even
if the collection still had entries.

This commit fixes the logic to decrement the `|entryCount` field by
the number of entries actually deleted. As an optimization, it is
still set directly to zero if `clear()` is not limited by patterns.

fixes #10007
@warner warner self-assigned this Aug 31, 2024
@mergify mergify bot closed this as completed in #10008 Aug 31, 2024
@mergify mergify bot closed this as completed in f8806f3 Aug 31, 2024
mergify bot added a commit that referenced this issue Aug 31, 2024
The `.clear()` method, on (strong) collections, will delete all entries. But it can also be called with `keyPatt` and/or `valuePatt` argument, and only the matching entries will be deleted.

Previously, the `|entryCount` metadata field was unconditionally set to zero for any call to `.clear()`, even those with patterns which caused some entries to be retained. This caused a subsequent `collection.getSize()` to return the wrong value, yielding zero even if the collection still had entries.

This commit fixes the logic to decrement the `|entryCount` field by the number of entries actually deleted. As an optimization, it is still set directly to zero if `clear()` is not limited by patterns.

fixes #10007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant