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

Is this client considred thread-safe ? #46

Open
sheinbergon opened this issue Mar 2, 2017 · 5 comments
Open

Is this client considred thread-safe ? #46

sheinbergon opened this issue Mar 2, 2017 · 5 comments

Comments

@sheinbergon
Copy link

Can I share a single Vault instance between multiple threads ?

@steve-perkins
Copy link
Contributor

The Vault class should be threadsafe in most common use cases. However, strictly speaking is not... and I would recommend not treating it as such, unless you're comfortable that your team would be disciplined enough to avoid the edge cases.

Vault has only one member variable, vaultConfig, which itself is final. The same is true for all of the API wrapper classes that Vault returns (e.g. Logical, Debug, etc.).

However, being final in Java does NOT mean immutable! You could still have multiple threads making changes to this config object's internals concurrently, and that would cause problems for sure.

The VaultConfig class uses the builder pattern, specifically to discourage mutating an instance after it's initially built. However, people COULD do it if they wanted to.

More importantly, Vault itself mutates its config when you call withRetries(...). This would probably be the biggest risk, because I could see users applying different retry settings to different API calls.

So.... IF you are not mutating your VaultConfig objects after building them, and IF you are not calling Vault.withRetries(...) from different places in your code with different values... then you are probably threadsafe in practice. However, we instantiate a new Vault for every thread, and that's the pattern I would recommend.

If there's demand for making Vault threadsafe, then we might look at modifying its internals with locks to prevent concurrent modification. However, my hunch is that the overhead of managing the locks would hurt performance more than the overhead of object creation (I could be wrong).

@sheinbergon
Copy link
Author

@steve-perkins thanks for responding. The above usage pattern sound reasonable, VaultConfig instance mutation should be handled only during client setup.

As for the withRetries aspect - well, it sounds like a common use pattern and having that be thread-unsafe is not that good. There might be some other way around it with synchronization/object recreation, but that'll require some refactoring. Would you be open to that ?

@steve-perkins
Copy link
Contributor

steve-perkins commented Apr 13, 2017

Sure! If you have any design proposals or want to try putting together a PR, I'd love to go over it. It may be a week or before I could get to it personally.

The solution may be as simple as adding synchronized to all of the VaultConfig builder methods (or at least the setMaxRetries(...) and setRetryIntervalMilliseconds(...) mutators that are called by Vault.withRetries(...)).

It just worries me that these VaultConfig objects are passed by Vault to all of the API wrappers, and stored there too. Then again, I suppose it's the same object reference shared by all them...

@NathanC
Copy link
Contributor

NathanC commented Jun 7, 2018

@steve-perkins Has your answer on this changed since you made it? Are there any other potential edge-conditions in using this between threads?

Depending on your answer, I could potentially open a PR to make it fully thread safe, or open a PR to document any concurrency considerations.

Thanks in advance! This looks like a great client.

@steve-perkins
Copy link
Contributor

Thanks, @NathanC. I'm certainly OPEN to the idea of making it possible and safe to swap out a Vault instance's VaultConfig object after instantiation... even if I'd probably never do so in my own code.

Either way, I would be extremely appreciative of any investigation or documentation around other concurrency considerations in general. I personally just don't share Vault instances between threads, so it hasn't been an issue for me or my company. In my original answer above, when I said that I don't think there are any considerations beyond the VaultConfig instance, that was an educated guess without any real legwork behind it.

@steve-perkins steve-perkins reopened this Jun 8, 2018
jentfoo pushed a commit to jentfoo/vault-java-driver that referenced this issue Jun 13, 2019
Deprecates the App ID backend.  Adds a new version of the Userpass backend.
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

No branches or pull requests

3 participants