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: use credentials key in pool #430

Merged
merged 3 commits into from Sep 29, 2020
Merged

fix: use credentials key in pool #430

merged 3 commits into from Sep 29, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Sep 15, 2020

The SpannerPool uses among other fields the credentials of a ConnectionOptions to determine whether an existing Spanner instance can be reused or not. Certain credentials instances, most notably UserCredentials, are mutable and the result of equals(Object) and hashCode() of an instance may change during its lifetime. This could cause the SpannerPool to inadvertently close a Spanner instance that is still in use.

This change replaces the usage of the actual credentials as part of the pool key with a key that is based on the method that was used to determine the credentials. This ensures that if two connections request a Spanner instance using the same credentialsUrl, that they will be considered equal. The same also applies to two connections that both do not set any specific credentials options and use the application default credentials.

Fixes googleapis/java-spanner-jdbc#206 and GoogleCloudPlatform/google-cloud-spanner-hibernate#202

@olavloite olavloite requested review from thiagotnunes and skuruppu Sep 15, 2020
@google-cla google-cla bot added the cla: yes label Sep 15, 2020
@product-auto-label product-auto-label bot added the api: spanner label Sep 15, 2020
@codecov
Copy link

@codecov codecov bot commented Sep 15, 2020

Codecov Report

Merging #430 into master will increase coverage by 0.06%.
The diff coverage is 91.30%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #430      +/-   ##
============================================
+ Coverage     82.16%   82.23%   +0.06%     
- Complexity     2455     2465      +10     
============================================
  Files           136      138       +2     
  Lines         13589    13620      +31     
  Branches       1307     1309       +2     
============================================
+ Hits          11166    11200      +34     
+ Misses         1895     1892       -3     
  Partials        528      528              
Impacted Files Coverage Δ Complexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java 86.98% <88.23%> (+0.44%) 30.00 <0.00> (ø)
...le/cloud/spanner/connection/ConnectionOptions.java 86.38% <100.00%> (+0.67%) 60.00 <2.00> (+3.00)
...m/google/cloud/spanner/spi/v1/GapicSpannerRpc.java 82.03% <0.00%> (ø) 81.00% <0.00%> (ø%)
...m/google/cloud/spanner/LazySpannerInitializer.java 50.00% <0.00%> (ø) 1.00% <0.00%> (?%)
.../google/cloud/spanner/AbstractLazyInitializer.java 100.00% <0.00%> (ø) 5.00% <0.00%> (?%)
...gle/cloud/spanner/AsyncTransactionManagerImpl.java 76.27% <0.00%> (+6.77%) 12.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b312091...10928e7. Read the comment docs.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Sep 28, 2020

@skuruppu @thiagotnunes Friendly ping for this PR. It fixes a bug in the JDBC driver that can cause a Pooled has been closed exception to occur.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Sep 28, 2020

hey @olavloite, I think I am missing how does assigning the credentials to a new field fixedCredentials will prevent mutability (since equals will compare the new CredentialsKey, which might have a credential, which might be mutable).

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Sep 28, 2020

hey @olavloite, I think I am missing how does assigning the credentials to a new field fixedCredentials will prevent mutability (since equals will compare the new CredentialsKey, which might have a credential, which might be mutable).

Thanks for the quick response @thiagotnunes

Assigning the credentials to the new field fixedCredentials will not fix the problem of mutability of the fixedCredentials. The problem however was that the methods of assigning a credentials instance and a credentialsUrl to the ConnectionOptions were not separated properly.

You have a number of options for setting the credentials when creating a ConnectionOptions instance:

  1. You can assign an OAuthToken that you have previously obtained.
  2. You can assign a Credentials instance.
  3. You set set a URL that points to a file that contains the credentials.
  4. You can let the client library pick the default credentials of the environment by not setting any of the above options.

Option 2 and 3 internally both set the ConnectionOptions.credentials field and when determining whether a Spanner instance was present in the pool, this field was used to determine equality. This change separates the two options so that:

  1. When a credentials instance has been assigned to the ConnectionOptions the field fixedCredentials will be used for the equality check.
  2. When a credentialsUrl has been set on the ConnectionOptions, the URL will be used for the equality check.

Setting a credentials instance directly when creating a ConnectionOptions will still suffer from any possible mutability of the credentials instance (or use of mutable fields in its equals method). It is however not currently used by any part of the client library or JDBC driver.

Setting a credentialsUrl is used by the JDBC driver and will no longer suffer from the possible mutability of the credentials that are chosen.

What also might be worth noting:

  • This problem occurs when using UserCredentials, as this class calls super.equals(Object) as part of its equals(Object) method. The super implementation (OAuth2Credentials) will compare the current OAuth tokens of this and other.
  • This problem does not occur when using ServiceAccountCredentials, as this class does not include any mutable fields in the equals(Object) implementation, and does not call the super implementation.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Sep 28, 2020

@olavloite thanks for the detailed explanation. LGTM.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Sep 28, 2020

@thiagotnunes Apparently you have not (yet?) been added as a member of https://github.com/orgs/googleapis/teams/yoshi-java/members, which means that merging this PR is still blocked until a member of that team approves the PR. Maybe one of the maintainers could add you as a member to the team.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Sep 28, 2020

@olavloite sorry about that, I have requested membership

elharo
elharo approved these changes Sep 29, 2020
@olavloite olavloite merged commit 28103fb into master Sep 29, 2020
18 checks passed
@olavloite olavloite deleted the use-credentials-key branch Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants