Skip to content

Make OAuth2Credentials#refreshTask volatile#1046

Closed
petedmarsh wants to merge 1 commit intogoogleapis:mainfrom
petedmarsh:make-refresh-task-volatile
Closed

Make OAuth2Credentials#refreshTask volatile#1046
petedmarsh wants to merge 1 commit intogoogleapis:mainfrom
petedmarsh:make-refresh-task-volatile

Conversation

@petedmarsh
Copy link
Copy Markdown

The synchronization on the lock is not enough to ensure that all threads have latest value of "refreshTask" which means it is possible for two refresh tasks to run at once:

// OAuth2Credentials#L247
synchronized (lock) {
  if (refreshTask != null) {
    return new AsyncRefreshResult(refreshTask, false);
  }

This is similar to why this.value has to be volatile.

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.

@petedmarsh petedmarsh requested a review from a team October 7, 2022 11:19
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Oct 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label Bot added the size: xs Pull request size is extra small. label Oct 7, 2022
The synchronization on the lock is not enough to ensure that all threads
have latest value of "refreshTask" which means it is possible for two
refresh tasks to run at once:

    // OAuth2Credentials#L247
    synchronized (lock) {
      if (refreshTask != null) {
        return new AsyncRefreshResult(refreshTask, false);
      }

This is similar to why this.value has to be volatile.
@petedmarsh petedmarsh force-pushed the make-refresh-task-volatile branch from c7b1ad2 to 63565ae Compare October 7, 2022 11:22
@petedmarsh
Copy link
Copy Markdown
Author

@petedmarsh petedmarsh closed this Oct 7, 2022
@petedmarsh petedmarsh deleted the make-refresh-task-volatile branch October 7, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: xs Pull request size is extra small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant