Introduced timeouts for MSAL calls. #2562
Conversation
machavan
requested review from
lilgreenbird,
tkyc,
David-Engel,
divang and
codewatchzen
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
============================================
+ Coverage 51.03% 51.14% +0.10%
- Complexity 3919 3927 +8
============================================
Files 147 147
Lines 33456 33478 +22
Branches 5604 5609 +5
============================================
+ Hits 17074 17122 +48
+ Misses 13971 13943 -28
- Partials 2411 2413 +2 ☔ View full report in Codecov by Sentry. |
src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java
Outdated
Show resolved
Hide resolved
| @@ -6110,10 +6110,11 @@ private SqlAuthenticationToken getFedAuthToken(SqlFedAuthInfo fedAuthInfo) throw | |||
| } | |||
|
|
|||
| while (true) { | |||
| int millisecondsRemaining = timerRemaining(timerExpire); | |||
There was a problem hiding this comment.
If this is 0 or negative does it make sense to try the calls below but not throw timed out exception here (or from timerRemaining call)? Or just let the first calls throw the exception?
There was a problem hiding this comment.
there's min value of 1ms, but the question is still valid.
There was a problem hiding this comment.
right, we are just letting the first call throw it
- Added more tests - Improved test to check for specific error message
- Replaced lock with tryLock to avoid potential long waiting for other threads while one thread is taking long to complete.
| try { | ||
| //Just try to acquire the lock and if can't then proceed to attempt to get the token | ||
| lockAcquired = lock.tryLock(Math.min(millisecondsRemaining, TOKEN_LOCK_WAIT_DURATION_MS), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Where are we using lockAcquired? We should go ahead only if it returns true(lock acquired), right?
There was a problem hiding this comment.
The idea is to attempt acquiring the lock with a timeout of 5 seconds. If the lock cannot be acquired within this time, proceed to attempt token acquisition regardless of whether the earlier thread(s) have released the lock or not
lockAcquired is used to check in the finally block if the lock should be released or not.
There was a problem hiding this comment.
So if it is ok to run 2 threads in the block why even attempt to check lock? It is either ok to run 2 or more threads or it is not. If ok (and the lack of checking the result of tryLock shows it is ok) then do not do any locking just run.
There was a problem hiding this comment.
The purpose is to optimize the token acquisition process, the first caller succeeding does caching which is then leveraged by subsequent threads. However, if the first thread takes considerable time, then we want the others to also go and try after waiting for a while.
If we were to let say 30 threads try in parallel, they would all miss the cache and hit the AAD auth endpoints to get their tokens at the same time, stressing the auth endpoint.
There was a problem hiding this comment.
Understood the intent, but would Semaphore be a more readable solution?
There was a problem hiding this comment.
Agree. The code has been updated along with detailed comment describing the intent.
- Added detailed comment for the usage of semaphore.
Description:
Calls to MSAL authentication library done from the Java driver code use future.get(), potentially leading to hangs in case the library does not receive timely response from the server. All these calls done as part of getFedAuthToken need to have timeouts imposed. The timeout period is calculated based on remaining time wrt login timeout, and passed to future.get(, ) calls with this fix.
Tests: