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

Oauth2 token race condition during parallel request #55

Open
kamijin-fanta opened this issue Dec 1, 2021 · 1 comment
Open

Oauth2 token race condition during parallel request #55

kamijin-fanta opened this issue Dec 1, 2021 · 1 comment

Comments

@kamijin-fanta
Copy link

Thanks for publishing an easy-to-use client library! I was wondering about the API design for actual use.

In the current code, it seems that oauth2.TokenSource is created for each request.

tokenSource := c.config.Oauth2.TokenSource(ctx, oauth2Token)

The internal implementation of oauth2.TokenSource , oauth2.reuseTokenSource , checks to see if the token is currently valid, and if not, it reclaims and updates the token. It is then serialized by Mutex.
https://github.com/golang/oauth2/blob/d3ed0bb246c8d3c75b63937d9a5eecff9c74d7fe/oauth2.go#L295-L310

With the current design of this client library, I believe there is a possibility of multiple refreshes and race conditions when parallel requests are made. This problem can be solved by changing the function signature to the following and passing the TokenSource from outside. (I understand that this is a big change.)

func (c *Client) GetUsersMe(ctx context.Context, tokenSource *oauth2.TokenSource, opts GetUsersMeOpts) (*Me, error)

tokenSource := config.Oauth2.TokenSource(ctx, token)
me, err := client.GetUsersMe(ctx, tokenSource, freee.GetUsersMeOpts{})

What are your thoughts on these issues?

@yyoshiki41
Copy link
Contributor

@kamijin-fanta
Sorry for late reply 🙏

Yes, you are correct. This library does not support parallel updates.
If the Token is updated at the same time, one request will be updated in the state of the old Token .
(and TokenSource should be concurrency-safe because it called at Transport.RoundTrip())

We plan to change the function signature when updating a major version in the near future.

Thanks for your reporting!!

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

2 participants