Improve active and inactive access token retrieval queries for MSSQL when con-app-key violation occurred

[sadilchamishka]

Proposed changes in this pull request

When con-app-key constrain violation occurred, following two queries get executed.
As per the analysis done for the MSSQL database, we can notice that it takes considerable amount of time to invoke the both queries (roughly 300ms per each).

The reason for the latency is due to the absence of NO LOCK syntax in the queries.

We have been using the same pattern when checking the existence of a token[2],[3] but we have missed in this place.

[1] - https://github.com/wso2-extensions/identity-inbound-auth-oauth/blob/master/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/AccessTokenDAOImpl.java#L2598~L2605
[2] -

identity-inbound-auth-oauth/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/SQLQueries.java

Line 205 in 9db893b

"CONSENTED_TOKEN FROM IDN_OAUTH2_ACCESS_TOKEN WITH (NOLOCK) WHERE CONSUMER_KEY_ID = (SELECT ID FROM " +

[3] -

identity-inbound-auth-oauth/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/SQLQueries.java

Line 213 in 9db893b

"IDN_OAUTH2_ACCESS_TOKEN WITH (NOLOCK) WHERE CONSUMER_KEY_ID = (SELECT ID FROM IDN_OAUTH_CONSUMER_APPS " +

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.91%. Comparing base (9db893b) to head (9964922).

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2728      +/-   ##
============================================
- Coverage     56.91%   56.91%   -0.01%     
+ Complexity     8581     8569      -12     
============================================
  Files           654      654              
  Lines         48442    48442              
  Branches      10263    10263              
============================================
- Hits          27573    27572       -1     
  Misses        16962    16962              
- Partials       3907     3908       +1     

Flag	Coverage Δ
unit	39.96% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[indeewari]

When con_app_key is supposed to prevent persisting the same token by multiple threads by synchronizing, how could removing this search lock not lead to data inconsistency ? I think this will increase write operations.

[sadilchamishka]

There is no point of having a read lock for the access-token table. Even we retrieve a data with a read lock, when we take the decision and take the action, the state might have been changed and again fail with con-app-key constraint violation.

The con-app-key violation reaching max attempt (default value 5) will be anyway reach having this improvement or not, but this improvement decrease the time to of reaching to the max failure attempts.

This limitation is only for the SQL server setups, as other databases works in the read with no lock status.

[Thumimku]

I agree that using a NOLOCK query will increase concurrent reads, but since the AT table is crucial, I am thinking whether allowing dirty reads is a tradeoff we can consider for performance improvement?

[sadilchamishka]

The dirty read happens for other database types also. This approach has been taken by SQL server as its default behaviour. If @Thumimku your argument is correct we have to fix the queries for other database types.

There is no any gain we read a data with a lock. Here we interested in check the existence. It just an idea that we are ready to do an insert. It is not an guarantee that we can do the insert. Hence a dirty read is fine for such a usecase.