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

fix DataCache functions have inconsistent behaviour #1193

Closed
wants to merge 4 commits into from
Closed

fix DataCache functions have inconsistent behaviour #1193

wants to merge 4 commits into from

Conversation

ixje
Copy link
Contributor

@ixje ixje commented Oct 28, 2019

fix #1186

@erikzhang
Copy link
Member

I don't think these changes are correct...

@ixje
Copy link
Contributor Author

ixje commented Oct 29, 2019

@erikzhang can you be more explicit what part you think is incorrect about it?

  1. This PR makes is so that all functions in all scenarios allow persisting to the real backend (as suggested in DataCache functions have inconsistent behaviour #1186).
  2. The keyCount values are different because TryGet() no longer produces NONE state trackables and thus in some scenarios we have an additional trackable returned from GetChangeSet()
  3. Because of point 2 the implementations of GetOrAdd() and GetAndChange() can be simplified by reusing existing code.

@erikzhang
Copy link
Member

Let's say we have a contract doing the following steps:

TryGet(key1)
TryGet(key2)
......
TryGet(key100)
GetAndChange(somekey)

It reads 100 entries, and update 1 entry. If we allow both TryGet() and GetAndChange() to persist data, we need to write to disk 101 times on commit. So we need TryGet() to keep entries being TrackState.None so that we write to disk only once on commit.

@ixje
Copy link
Contributor Author

ixje commented Oct 29, 2019

I agree it will attempt to modify 101 entries (assuming LevelDB doesn't do anything smart if it sees no changes). However, we need to keep in mind that everything is put into a WriteBatch and disk I/O happens only once when calling Commit() on the snapshot, not 101 times.

protected override void UpdateInternal(TKey key, TValue value)
{
batch?.Put(prefix, key, value);
}

My fear is that there is a security issue lurking around the corner because some people believe TryGet() is always read only. Example (#855 (comment)):

Because anything that you do with TryGet never will be committed, with GetAndChange, your changes will be applied

and this is not true as shown in this table (case 1 + 6)
Screenshot 2019-10-24 at 12 55 20
and the code example here: #855 (comment)

@shargon
Copy link
Member

shargon commented Nov 14, 2019

What about if we use a different dictionaries for caching TryGet (readonly) and the others?

Option B, if it's already in the cache, return a copy when use TryGet

@ixje
Copy link
Contributor Author

ixje commented Nov 18, 2019

Option B, a copy would streamline the behaviour in all situations.

Alternatively, an optional read_only parameter could be added. Then make TryGet results persistable by default, which is also how C# dictionaries work (for TryGetValue), and which also allows to re-use and simplify the code following this PR.

@lock9 lock9 changed the title fix 1186 fix DataCache functions have inconsistent behaviour Nov 21, 2019
@erikzhang erikzhang closed this Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataCache functions have inconsistent behaviour
4 participants