tough datastore: treat system_time as critical section and document #609
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
Add some documentation to the datastore in the tough library. Importantly, we now document the lack of thread safety in the reader function.
Because we are going to add another field to the Datastore struct, we want to use named fields instead of tuple syntax. This commit has no changes to behavior.
Move the system_time function into the Datastore object as a &self function.
stmcginnis
approved these changes
Apr 21, 2023
cbgbt
approved these changes
May 23, 2023
tough/src/datastore.rs
Outdated
| /// is protected by a lock guard to ensure thread safety. | ||
| pub(crate) fn system_time(&self) -> Result<DateTime<Utc>> { | ||
| // Treat this function as a critical section. This lock is not used for anything else. | ||
| let _lock = self.time_lock.write().map_err(|e| { |
There was a problem hiding this comment.
I always wondered if non-lexical lifetimes meant that this could potentially be dropped prior to the end of the lexical scope.
After reading a few resources, my understanding is that this will be dropped "at the end of its lifetime" which is currently the end of the scope. Possibly it's worth explicitly drop(_lock) at the end of the function to make accidental lifetime changes result in a compiler error.
Treat the datastore system_time function as a critical section by giving it its own lock guard.
cbgbt
approved these changes
May 25, 2023
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.
Issue #, if available:
Closes #604
Documents which Datastore functions are thread safe and which one is not, see #602
Description of changes:
Adds documentation to the Datastore, moves the system_time function into the Datastore, and adds a lock guard to ensure thread safety of that function.
Testing
I tested this with our internal monitoring system for Bottlerocket repos and it worked.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.