Fix underflow possibility on ColumnFamilyDbStore #5975
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nyanzebra
commented
Jan 10, 2022
edge-util/src/Microsoft.Azure.Devices.Edge.Storage.RocksDb/ColumnFamilyDbStore.cs
Outdated
Show resolved
Hide resolved
nyanzebra
commented
Jan 10, 2022
| @@ -103,5 +104,53 @@ public async Task MessageCountTest() | |||
| Assert.Equal(0ul, await columnFamilyDbStore.Count()); | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task MessageCountUnderflowTest() | |||
There was a problem hiding this comment.
Made a separate test altogether
nyanzebra
commented
Jan 10, 2022
edge-util/test/Microsoft.Azure.Devices.Edge.Storage.RocksDb.Test/ColumnFamilyStoreTest.cs
Outdated
Show resolved
Hide resolved
|
Will fix styling and other stuff once method of fix is approved above :) |
| @@ -75,7 +75,20 @@ public async Task Remove(byte[] key, CancellationToken cancellationToken) | |||
|
|
|||
| Action operation = () => this.db.Remove(key, this.Handle); | |||
| await operation.ExecuteUntilCancelled(cancellationToken); | |||
| Interlocked.Decrement(ref this.count); | |||
| // We don't want to wrap around the counter. | |||
| var currentCount = Interlocked.Read(ref this.count); | |||
There was a problem hiding this comment.
You will need to lock the entire block - if 3 threads read a value (say 2) and all 3 call decrement you will end up with -1.
There was a problem hiding this comment.
Yeah, I am really trying to avoid the lock.... perhaps the better thing to do is just let it be long and as spoke about offline to make the Metrics count for whatever it really is and just make sure the metric count is checked to make sure we avoid this problem
nimanch
reviewed
Jan 12, 2022
edge-util/test/Microsoft.Azure.Devices.Edge.Storage.RocksDb.Test/ColumnFamilyStoreTest.cs
Outdated
Show resolved
Hide resolved
|
Keeping this open until I have result from running for several days |
varunpuranik
approved these changes
Jan 13, 2022
nyanzebra
added a commit
to nyanzebra/iotedge
that referenced
this pull request
Jan 20, 2022
* Fix underflow * Use compare exchange and make separate test * Use Max for count instead of trying to make atomic check
nyanzebra
added a commit
to nyanzebra/iotedge
that referenced
this pull request
Jan 20, 2022
* Fix underflow * Use compare exchange and make separate test * Use Max for count instead of trying to make atomic check
lt72
pushed a commit
to lt72/iotedge
that referenced
this pull request
Jan 31, 2022
* Fix underflow * Use compare exchange and make separate test * Use Max for count instead of trying to make atomic check
nyanzebra
added a commit
that referenced
this pull request
Feb 1, 2022
nyanzebra
added a commit
that referenced
this pull request
Feb 1, 2022
seanyen
pushed a commit
to seanyen/iotedge
that referenced
this pull request
Feb 1, 2022
…6025) * Fix underflow * Use compare exchange and make separate test * Use Max for count instead of trying to make atomic check
damonbarry
pushed a commit
to damonbarry/iotedge
that referenced
this pull request
Apr 15, 2022
* Fix underflow * Use compare exchange and make separate test * Use Max for count instead of trying to make atomic check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue where in certain conditions (EH has disparate info from Checkpointer and MessageStore and can happen when crash occurs before complete updates) it may be possible to remove from the store and go below 0.
Updated existing test to make sure underflow will be caught.
Azure IoT Edge PR checklist:
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines and Best Practices
Testing Guidelines
Draft PRs
Draftmode if it is:Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned here for the PR title and description