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

Mutithreading issues in cognitoIdentity CognitoUser #272

Conversation

nikhilshekhawat
Copy link

fixed bug for when multiple threads were calling CognitoUser.getSessionInBackground. Currently there might be a scenario where multiple threads call this function and all of them go over the network to get the new ID token after the 1hour expiry. This error is for aws-sdk version 2.4.

In earlier version of the same SDK if we made multiple calls to this function one of the threads would just fail and require user login. This error is for aws-sdk version 2.2.

http://stackoverflow.com/questions/43104955/aws-cognito-getsessioninbackground-fails-when-id-token-needs-refreshing

…onInBackground, right now there might be a scenario where 10 threads call this function and all of them go over the network to get the new ID token after the 1hour expiry.
@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@1d492dc). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #272   +/-   ##
=========================================
  Coverage          ?    6.79%           
=========================================
  Files             ?     5009           
  Lines             ?   193826           
  Branches          ?    46509           
=========================================
  Hits              ?    13163           
  Misses            ?   179527           
  Partials          ?     1136
Impacted Files Coverage Δ
...onnectors/cognitoidentityprovider/CognitoUser.java 0% <0%> (ø)

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 1d492dc...7ac0976. Read the comment docs.

@mutablealligator
Copy link
Contributor

Hello @nikhilshekhawat,

Thanks for using our SDK and submitting a pull request. Can you resolve the conflicts and re-submit?

Thanks

@mutablealligator
Copy link
Contributor

Hi @nikhilshekhawat, Can you resolve the conflicts and re-submit the PR?

Copy link
Contributor

@minbi minbi left a comment

Choose a reason for hiding this comment

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

This looks like a good optimization

throw new CognitoNotAuthorizedException("User is not authenticated", nae);
} catch (Exception e) {
CognitoUser.semaphore.release();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are dealing with locking here, can you move the release into a finally block?

/**
* The semaphore to provide mutex for the getSession call if called on multiple threads, this would avoid multiple threads going over the network and refreshing the tokens
*/
public static Semaphore semaphore = new Semaphore(1, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is public? If not please change to private.

@frankmuellr frankmuellr added cognito Issues with the AWS Android SDK for Cognito pull request and removed cognito Issues with the AWS Android SDK for Cognito UserPools labels Sep 28, 2018
@mutablealligator
Copy link
Contributor

Addressing this PR changes in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cognito Issues with the AWS Android SDK for Cognito
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants