fix: Resolve race condition reported in #692 #1031
Conversation
4434bc4 to
e0b5cfa
Compare
johanblumenberg
left a comment
There was a problem hiding this comment.
looks good
I tried it in our product as well and it seems to work
|
@johanblumenberg @igorbernstein2 @TimurSadykov I've also been affected by this race condition and I would like to see a fix (this one looks along the right lines to me at least), but Edit: making it volatile is not required, the synchronized block is enough. |
65547c7 to
a7299bf
Compare
| this.listener = listener; | ||
|
|
||
| // Update Credential state first | ||
| task.addListener(listener, MoreExecutors.directExecutor()); |
There was a problem hiding this comment.
I think you should either in-line both callbacks or move all the callback logic into the listener class.
There was a problem hiding this comment.
In order to inject a sleep to exercise the race condition I wrote it like this. Agreed the logic could be better but I am not very familiar with Java
There was a problem hiding this comment.
That said I would happily accept a cleaner approach!
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java
Outdated
Show resolved
Hide resolved
TimurSadykov
left a comment
There was a problem hiding this comment.
LGTM, forgot to post few nit comments
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [1.12.1](https://togithub.com/googleapis/google-auth-library-java/compare/v1.12.0...v1.12.1) (2022-10-18) ### Bug Fixes * Resolve race condition reported in [#692](https://togithub.com/googleapis/google-auth-library-java/issues/692) ([#1031](https://togithub.com/googleapis/google-auth-library-java/issues/1031)) ([87a6606](https://togithub.com/googleapis/google-auth-library-java/commit/87a66067dff49d68f5b01cfe4c3f755fbf6b44fb)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
|
It seems I've been also affected by this. What exactly I should update? I don't see neither google-auth-library nor com.google.auth in my dep. tree. |
|
@nnl-1 Please share what kind of client are you using where you see the issue and how you check the dependency tree? |
@TimurSadykov I'm using this dep. https://mvnrepository.com/artifact/com.google.cloud.sql/postgres-socket-factory. The dep. tree command is 'mvn dependency:tree'. |
|
ok, now I see it — GoogleCloudPlatform/cloud-sql-jdbc-socket-factory@c6df99f. Thanks |
Fork of #1002 with test fixes.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.