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

Add thread-safe cache #84

Merged
merged 1 commit into from
Feb 21, 2021
Merged

Add thread-safe cache #84

merged 1 commit into from
Feb 21, 2021

Conversation

fishi0x01
Copy link
Owner

@fishi0x01 fishi0x01 commented Feb 21, 2021

  • replaces all thread-unsafe caching mechanisms with a thread safe cache
  • add a vault test with many secrets, for better concurrency testing

Having a thread safe cache will make concurrency implementation easier.

Fixes #82

@fishi0x01 fishi0x01 added the enhancement New feature or request label Feb 21, 2021
@fishi0x01 fishi0x01 self-assigned this Feb 21, 2021
@fishi0x01 fishi0x01 force-pushed the 82_query-cache branch 3 times, most recently from ffeaeb1 to e3d95e9 Compare February 21, 2021 12:06
@fishi0x01
Copy link
Owner Author

Performance test extends CI test time too much. Will adjust it to run only once and not on each KV.

@fishi0x01
Copy link
Owner Author

Concurrency tests now only run on single vault and KV version to save time.

Copy link
Collaborator

@mattlqx mattlqx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 This looks like a great change I can refactor my pull around.

client/cache.go Outdated
if value, ok := cache.listQueries[path]; ok {
result = value.Secret
err = value.Err
hit = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't ok be used for this?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍
Adjusted it now

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, apparently this doesn't work:

var hit bool
if value, hit := cache.listQueries[path]; hit {
    result = value.Secret
    err = value.Err
}
if hit {
    // this branch is never reached
    return result, err
}

Just now verified that it results in hit always being false after leaving the first if scope. I am not really sure why though

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the assignment is considered in the scope of the if because of the :=? These little Golang things catch me out too. :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this works :)

value, hit := cache.listQueries[path]
if hit {
    return value.Secret, value.Err
}

I think this part ; ok { binds the two initialized variables solely to the if scope.

@fishi0x01 fishi0x01 force-pushed the 82_query-cache branch 3 times, most recently from 5d0d973 to c915bf9 Compare February 21, 2021 17:46
@fishi0x01
Copy link
Owner Author

@mattlqx Concerning concurrency:
I have another branch based on this PR, which parallelizes the Traverse and runCommandWithTraverseTwoPaths functions. It seems like mv, rm and cp can benefit quite well from it. replace and grep do not benefit from it though, because they use a different traversal logic - however, that can be adjusted.
Will open that PR soon

@fishi0x01 fishi0x01 merged commit af2a284 into master Feb 21, 2021
@fishi0x01 fishi0x01 deleted the 82_query-cache branch February 21, 2021 18:09
@fishi0x01
Copy link
Owner Author

#86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proper cache for recursive operations
2 participants