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.

Sign up for GitHub

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

Jump to bottom

fix: Resolve race condition reported in #692 #1031

Merged
merged 19 commits into from
Oct 18, 2022
Merged

fix: Resolve race condition reported in #692 #1031

merged 19 commits into from
Oct 18, 2022

Conversation

clundin25
Copy link
Contributor

@clundin25 clundin25 commented Sep 29, 2022 •
edited

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@clundin25 clundin25 requested a review from a team as a code owner September 29, 2022 18:41
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Sep 29, 2022
@clundin25 clundin25 mentioned this pull request Sep 29, 2022
Closed
4 tasks
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Sep 30, 2022
johanblumenberg
johanblumenberg approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@johanblumenberg johanblumenberg left a comment

Choose a reason for hiding this comment

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

looks good

I tried it in our product as well and it seems to work

@petedmarsh
Copy link

petedmarsh commented Oct 7, 2022 •
edited

@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 I think there may be one thing missing which is making the refreshTask volatile too as in:

#1046

Edit: making it volatile is not required, the synchronized block is enough.

igorbernstein
igorbernstein reviewed Oct 14, 2022
View reviewed changes
oauth2_http/java/com/google/auth/oauth2/OAuth2Credentials.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/OAuth2Credentials.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/OAuth2Credentials.java
this.listener = listener;

// Update Credential state first
task.addListener(listener, MoreExecutors.directExecutor());
Copy link

Choose a reason for hiding this comment

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

I think you should either in-line both callbacks or move all the callback logic into the listener class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
TimurSadykov approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

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
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java Show resolved Hide resolved
@clundin25 clundin25 merged commit 87a6606 into googleapis:main Oct 18, 2022
@clundin25 clundin25 mentioned this pull request Oct 18, 2022
Closed
@release-please release-please bot mentioned this pull request Oct 18, 2022
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Oct 18, 2022
@release-please
chore(main): release 1.12.1 (#1059)
382e96a 
🤖 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).
@nnl-1
Copy link

nnl-1 commented Oct 26, 2022

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.

@TimurSadykov
Copy link
Member

@nnl-1 Please share what kind of client are you using where you see the issue and how you check the dependency tree?

@nnl-1
Copy link

nnl-1 commented Oct 28, 2022 •
edited

@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'.

@nnl-1
Copy link

nnl-1 commented Nov 1, 2022

ok, now I see it — GoogleCloudPlatform/cloud-sql-jdbc-socket-factory@c6df99f. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorbernstein igorbernstein igorbernstein left review comments

@TimurSadykov TimurSadykov TimurSadykov approved these changes

@johanblumenberg johanblumenberg johanblumenberg approved these changes

@igorbernstein2 igorbernstein2 Awaiting requested review from igorbernstein2

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@clundin25 @petedmarsh @nnl-1 @TimurSadykov @igorbernstein @johanblumenberg @igorbernstein2