-
Notifications
You must be signed in to change notification settings - Fork 330
Connect timeout propagated through pool #4270
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
d37911c
cdbac4a
8abc9d5
d8cb7f9
c2658d3
57708b3
c8342a9
634f350
3a7fa1e
fc9e85f
c9cf6ef
d6a26e3
220e945
03ebdf0
ad0649d
635ddcf
c18c227
7e241b1
fed27de
aad4c94
21f5696
535e242
16494ad
dc7bfa1
0ed523e
df97c19
0358280
e0161df
70db022
d8d7127
16d1133
fcb6acf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,151 +5,108 @@ | |
| using Microsoft.Data.Common; | ||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Threading; | ||
|
|
||
| namespace Microsoft.Data.ProviderBase | ||
| { | ||
| // Purpose: | ||
| // Manages determining and tracking timeouts | ||
| // | ||
| // Intended use: | ||
| // Call StartXXXXTimeout() to get a timer with the given expiration point | ||
| // Get remaining time in appropriate format to pass to subsystem timeouts | ||
| // Check for timeout via IsExpired for checks in managed code. | ||
| // Simply abandon to GC when done. | ||
| /// <summary> | ||
| /// Manages determining and tracking timeouts for use by subsystems that perform | ||
| /// time-bounded operations. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// Intended use: | ||
| /// </para> | ||
| /// <para> | ||
| /// Call <see cref="StartNew"/> to get a timer with the given expiration point. | ||
| /// Read the remaining time in the appropriate format to pass to subsystem timeouts. | ||
| /// Check for timeout via <see cref="IsExpired"/> for checks in managed code. | ||
| /// Simply abandon the instance to the GC when done. | ||
| /// </para> | ||
| /// </remarks> | ||
| internal class TimeoutTimer | ||
| { | ||
| //------------------- | ||
| // Fields | ||
| //------------------- | ||
| private long _timerExpire; | ||
| private bool _isInfiniteTimeout; | ||
| private long _originalTimerTicks; | ||
|
|
||
| //------------------- | ||
| // Timeout-setting methods | ||
| //------------------- | ||
|
|
||
| // Get a new timer that will expire in the given number of seconds | ||
| // For input, a value of zero seconds indicates infinite timeout | ||
| internal static TimeoutTimer StartSecondsTimeout(int seconds) | ||
| { | ||
| //-------------------- | ||
| // Preconditions: None (seconds must conform to SetTimeoutSeconds requirements) | ||
| #region Fields | ||
|
|
||
| //-------------------- | ||
| // Method body | ||
| var timeout = new TimeoutTimer(); | ||
| timeout.SetTimeoutSeconds(seconds); | ||
| /// <summary> | ||
| /// The sentinel value (<c>0</c>) used to indicate an infinite timeout when starting a timer. | ||
| /// </summary> | ||
| internal static readonly long InfiniteTimeout = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| //--------------------- | ||
| // Postconditions | ||
| Debug.Assert(timeout != null); // Need a valid timeouttimer if no error | ||
| #endregion | ||
|
|
||
| return timeout; | ||
| } | ||
| #region Constructors | ||
|
|
||
| // Get a new timer that will expire in the given number of milliseconds | ||
| // No current need to support infinite milliseconds timeout | ||
| internal static TimeoutTimer StartMillisecondsTimeout(long milliseconds) | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="TimeoutTimer"/> class with the | ||
| /// specified expiration duration. | ||
| /// </summary> | ||
| /// <param name="expiration"> | ||
| /// The duration before the timer expires. A value whose ticks equal | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the use case for a Also, I think
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's to provide a standard timeout check across the whole stack. Otherwise we would need to pass a timeout and an isInfinite flag. |
||
| /// <see cref="InfiniteTimeout"/> indicates an infinite timeout. | ||
| /// </param> | ||
| private TimeoutTimer(TimeSpan expiration) | ||
| { | ||
| //-------------------- | ||
| // Preconditions | ||
| Debug.Assert(0 <= milliseconds); | ||
|
|
||
| //-------------------- | ||
| // Method body | ||
| var timeout = new TimeoutTimer(); | ||
| timeout._originalTimerTicks = milliseconds * TimeSpan.TicksPerMillisecond; | ||
| timeout._timerExpire = checked(ADP.TimerCurrent() + timeout._originalTimerTicks); | ||
| timeout._isInfiniteTimeout = false; | ||
|
|
||
| //--------------------- | ||
| // Postconditions | ||
| Debug.Assert(timeout != null); // Need a valid timeouttimer if no error | ||
|
|
||
| return timeout; | ||
| OriginalTicks = expiration.Ticks; | ||
| IsInfinite = OriginalTicks == InfiniteTimeout; | ||
| ExpirationTicks = IsInfinite ? long.MaxValue : checked(ADP.TimerCurrent() + OriginalTicks); | ||
|
mdaigle marked this conversation as resolved.
Outdated
mdaigle marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| //------------------- | ||
| // Methods for changing timeout | ||
| //------------------- | ||
| #endregion | ||
|
|
||
| internal void SetTimeoutSeconds(int seconds) | ||
| { | ||
| //-------------------- | ||
| // Preconditions | ||
| Debug.Assert(0 <= seconds || InfiniteTimeout == seconds); // no need to support negative seconds at present | ||
| #region Properties | ||
|
|
||
| //-------------------- | ||
| // Method body | ||
| if (InfiniteTimeout == seconds) | ||
| { | ||
| _isInfiniteTimeout = true; | ||
| } | ||
| else | ||
| { | ||
| // Stash current time + timeout | ||
| _originalTimerTicks = ADP.TimerFromSeconds(seconds); | ||
| _timerExpire = checked(ADP.TimerCurrent() + _originalTimerTicks); | ||
| _isInfiniteTimeout = false; | ||
| } | ||
|
|
||
| //--------------------- | ||
| // Postconditions:None | ||
| } | ||
|
|
||
| // Reset timer to original duration. | ||
| internal void Reset() | ||
| { | ||
| if (InfiniteTimeout == _originalTimerTicks) | ||
| { | ||
| _isInfiniteTimeout = true; | ||
| } | ||
| else | ||
| { | ||
| _timerExpire = checked(ADP.TimerCurrent() + _originalTimerTicks); | ||
| _isInfiniteTimeout = false; | ||
| } | ||
| /// <summary> | ||
| /// Gets the absolute tick value at which this timer is considered expired. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what When we use signed types for values that are naturally unsigned (why?), the negative question is always good to call out explicitly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess negative values are technically valid. I will remove |
||
| /// </summary> | ||
| /// <value> | ||
| /// The tick count, in <see cref="ADP.TimerCurrent"/> units, at which the timer | ||
| /// expires; <see cref="long.MaxValue"/> when <see cref="IsInfinite"/> is | ||
| /// <see langword="true"/>. | ||
| /// </value> | ||
| internal long ExpirationTicks { | ||
| get; | ||
| //TODO: Remove this when we disable Reset() | ||
| private set; | ||
| } | ||
|
|
||
| //------------------- | ||
| // Timeout info properties | ||
| //------------------- | ||
|
|
||
| // Indicator for infinite timeout when starting a timer | ||
| internal static readonly long InfiniteTimeout = 0; | ||
|
|
||
| // Is this timer in an expired state? | ||
| /// <summary> | ||
| /// Gets a value indicating whether this timer has expired. | ||
| /// </summary> | ||
| /// <value> | ||
| /// <see langword="true"/> if the timer is not infinite and the current time | ||
| /// has passed <see cref="ExpirationTicks"/>; otherwise, <see langword="false"/>. | ||
| /// </value> | ||
| internal bool IsExpired | ||
| { | ||
| get | ||
| { | ||
| return !IsInfinite && ADP.TimerHasExpired(_timerExpire); | ||
| return !IsInfinite && ADP.TimerHasExpired(ExpirationTicks); | ||
| } | ||
| } | ||
|
|
||
| // is this an infinite-timeout timer? | ||
| internal bool IsInfinite | ||
| { | ||
| get | ||
| { | ||
| return _isInfiniteTimeout; | ||
| } | ||
| } | ||
| /// <summary> | ||
| /// Gets a value indicating whether this timer represents an infinite timeout. | ||
| /// </summary> | ||
| /// <value> | ||
| /// <see langword="true"/> if the timer was created with an expiration whose | ||
| /// ticks equal <see cref="InfiniteTimeout"/>; otherwise, <see langword="false"/>. | ||
| /// </value> | ||
| internal bool IsInfinite { get; } | ||
|
|
||
| // Special accessor for TimerExpire for use when thunking to legacy timeout methods. | ||
| public long LegacyTimerExpire | ||
| { | ||
| get | ||
| { | ||
| return (_isInfiniteTimeout) ? long.MaxValue : _timerExpire; | ||
| } | ||
| } | ||
|
|
||
| // Returns milliseconds remaining trimmed to zero for none remaining | ||
| // and long.MaxValue for infinite | ||
| // This method should be preferred for internal calculations that are not | ||
| // yet common enough to code into the TimeoutTimer class itself. | ||
| /// <summary> | ||
| /// Gets the number of milliseconds remaining before this timer expires, | ||
| /// trimmed to <c>0</c> when none remain and to <see cref="long.MaxValue"/> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be more precise to say something like: ... truncated to 0 when < 1 ms remains. This will be MaxValue when IsInfinite is true, and may be MaxValue for a freshly started timer with a very large expiration. |
||
| /// when the timer is infinite. | ||
| /// </summary> | ||
| /// <value> | ||
| /// A non-negative count of milliseconds remaining; <see cref="long.MaxValue"/> | ||
| /// when <see cref="IsInfinite"/> is <see langword="true"/>. | ||
| /// </value> | ||
| /// <remarks> | ||
| /// This property should be preferred for internal calculations that are not | ||
| /// yet common enough to code into the <see cref="TimeoutTimer"/> class itself. | ||
| /// </remarks> | ||
| internal long MillisecondsRemaining | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Long term I would like refactor these to use TimeSpan, but changing these methods has a lot of downstream effects, so they remain as-is for now. |
||
| { | ||
| get | ||
|
|
@@ -160,13 +117,13 @@ internal long MillisecondsRemaining | |
| //------------------- | ||
| // Method Body | ||
| long milliseconds; | ||
| if (_isInfiniteTimeout) | ||
| if (IsInfinite) | ||
| { | ||
| milliseconds = long.MaxValue; | ||
| } | ||
| else | ||
| { | ||
| milliseconds = ADP.TimerRemainingMilliseconds(_timerExpire); | ||
| milliseconds = ADP.TimerRemainingMilliseconds(ExpirationTicks); | ||
| if (0 > milliseconds) | ||
| { | ||
| milliseconds = 0; | ||
|
|
@@ -181,21 +138,30 @@ internal long MillisecondsRemaining | |
| } | ||
| } | ||
|
|
||
| // Returns milliseconds remaining trimmed to zero for none remaining | ||
| /// <summary> | ||
| /// Gets the number of milliseconds remaining before this timer expires as | ||
| /// a 32-bit integer, trimmed to <c>0</c> when none remain and saturated to | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same precision comment here as above. |
||
| /// <see cref="int.MaxValue"/> when the remaining time exceeds that value or | ||
| /// when the timer is infinite. | ||
| /// </summary> | ||
| /// <value> | ||
| /// A non-negative count of milliseconds remaining, never exceeding | ||
| /// <see cref="int.MaxValue"/>. | ||
| /// </value> | ||
| internal int MillisecondsRemainingInt | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This property body could be simplified to:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the methods are so intricate, I opted to leave them untouched as much as possible to reduce changes. |
||
| { | ||
| get | ||
| { | ||
| //------------------- | ||
| // Method Body | ||
| int milliseconds; | ||
| if (_isInfiniteTimeout) | ||
| if (IsInfinite) | ||
| { | ||
| milliseconds = int.MaxValue; | ||
| } | ||
| else | ||
| { | ||
| long longMilliseconds = ADP.TimerRemainingMilliseconds(_timerExpire); | ||
| long longMilliseconds = ADP.TimerRemainingMilliseconds(ExpirationTicks); | ||
| if (0 > longMilliseconds) | ||
| { | ||
| milliseconds = 0; | ||
|
|
@@ -217,5 +183,74 @@ internal int MillisecondsRemainingInt | |
| return milliseconds; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the original timeout duration, in ticks, that was supplied when the | ||
| /// timer was created. Used by <see cref="Reset"/> to restore the original | ||
| /// expiration window. | ||
| /// </summary> | ||
| private long OriginalTicks { get; } | ||
|
|
||
| #endregion | ||
|
|
||
| #region Methods | ||
|
|
||
| /// <summary> | ||
| /// Creates and starts a new <see cref="TimeoutTimer"/> with the specified | ||
| /// expiration duration. | ||
| /// </summary> | ||
| /// <param name="expiration"> | ||
| /// The duration before the returned timer expires. A value whose ticks equal | ||
| /// <see cref="InfiniteTimeout"/> produces an infinite timer. | ||
| /// </param> | ||
| /// <returns>A new <see cref="TimeoutTimer"/> instance that has already started.</returns> | ||
| internal static TimeoutTimer StartNew(TimeSpan expiration) => new TimeoutTimer(expiration); | ||
|
|
||
| /// <summary> | ||
| /// Creates a new <see cref="CancellationTokenSource"/> that will be canceled | ||
| /// when this timer expires. | ||
| /// </summary> | ||
| /// <returns> | ||
| /// A <see cref="CancellationTokenSource"/> scheduled to cancel after | ||
| /// <see cref="MillisecondsRemainingInt"/> milliseconds. When | ||
| /// <see cref="IsInfinite"/> is <see langword="true"/>, the returned source | ||
| /// is never automatically canceled. When the timer has already expired, the | ||
| /// returned source is already canceled. | ||
| /// </returns> | ||
| internal CancellationTokenSource CreateCancellationTokenSource() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Within the pool, we need to generate a cancellation token that will cancel after the remaining on the timeout has expired. I'm not wild about this, but it feels complicated to remember to check if a timeout is infinite when creating a token source. If desired, I can move this inline in ChannelDbConnectionPool. That said, I think we may want this functionality available to us as we move ahead with other asynchronous pathway improvements.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems fine to have this class expose a CTS equivalent to the timeout it is tracking. Could we avoid repeatedly calculating this by creating it once during construction and converting this into a read-only property? Add a little memory overhead and remove repeated calculations. Or do we need to create one every time since the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can move it inside, but then we need to make TimeoutTimer IDisposable, which further complicates things. I don't think we can do disposal easily because we'll need to track when it's appropriate to dispose if we're in an async flow. Our async improvements that we tackle next will make things like this easier. |
||
| { | ||
| if (IsInfinite) | ||
| { | ||
| return new CancellationTokenSource(); | ||
| } | ||
|
|
||
| int remaining = MillisecondsRemainingInt; | ||
| if (remaining == 0) | ||
| { | ||
| CancellationTokenSource cts = new CancellationTokenSource(); | ||
| cts.Cancel(); | ||
| return cts; | ||
| } | ||
|
|
||
| return new CancellationTokenSource(remaining); | ||
| } | ||
|
mdaigle marked this conversation as resolved.
|
||
|
|
||
| /// <summary> | ||
| /// Resets the timeout to its original duration. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This method is only used to retry after federated authentication timeouts, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting... would a Although, isn't a plain reset equivalent to IMO these objects should be immutable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should definitely be immutable and I want to remove the reset flow entirely. That said, today, the effect of the rest is observed by downstream code. If we "reset" by creating a new instance, it won't impact other code that still references the old instance. |
||
| /// which can use up the whole timeout due to MFA. Has no effect when | ||
| /// <see cref="IsInfinite"/> is <see langword="true"/>. | ||
| /// </remarks> | ||
| internal void Reset() | ||
| { | ||
| if (!IsInfinite) | ||
| { | ||
| ExpirationTicks = checked(ADP.TimerCurrent() + OriginalTicks); | ||
| } | ||
| } | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,6 +317,7 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable | |
| internal SqlConnectionInternal( | ||
| DbConnectionPoolIdentity identity, | ||
| SqlConnectionOptions connectionOptions, | ||
| TimeoutTimer timeout, | ||
| SqlCredential credential, | ||
| DbConnectionPoolGroupProviderInfo providerInfo, | ||
| string newPassword, | ||
|
|
@@ -399,8 +400,8 @@ internal SqlConnectionInternal( | |
|
|
||
| try | ||
| { | ||
| _timeout = TimeoutTimer.StartSecondsTimeout(connectionOptions.ConnectTimeout); | ||
|
|
||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The TimeoutTimer now comes from the caller, so we don't need to create one here. |
||
| _timeout = timeout; | ||
| // If transient fault handling is enabled then we can retry the login up to the | ||
| // ConnectRetryCount. | ||
| int connectionEstablishCount = applyTransientFaultHandling | ||
|
|
@@ -2204,6 +2205,7 @@ private void AttemptOneLogin( | |
| /// if a cached token exists from a previous auth attempt (see GetFedAuthToken). | ||
| /// </summary> | ||
| // @TODO: Rename to meet naming conventions | ||
| // TODO: if this call timed out, what reason do we have to believe some other call succeeded? why not just fail? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the flow where we want Interactive logins to succeed even though the ConnectTimeout has passed. It might take some time (more than 15 seconds) for the user to type in their username, pull out their phone, get a code, type it in, and then get a token. |
||
| private bool AttemptRetryADAuthWithTimeoutError( | ||
| SqlException sqlex, | ||
| SqlConnectionOptions connectionOptions, // @TODO: this is not used | ||
|
|
@@ -3216,7 +3218,7 @@ private void LoginNoFailover( | |
| { | ||
| nextTimeoutInterval = milliseconds; | ||
| } | ||
|
mdaigle marked this conversation as resolved.
Outdated
|
||
| intervalTimer = TimeoutTimer.StartMillisecondsTimeout(nextTimeoutInterval); | ||
| intervalTimer = TimeoutTimer.StartNew(TimeSpan.FromMilliseconds(nextTimeoutInterval)); | ||
|
mdaigle marked this conversation as resolved.
Outdated
mdaigle marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| // Re-allocate parser each time to make sure state is known. | ||
|
|
@@ -3501,7 +3503,7 @@ private void LoginWithFailover( | |
| nextTimeoutInterval = milliseconds; | ||
| } | ||
|
|
||
|
mdaigle marked this conversation as resolved.
|
||
| TimeoutTimer intervalTimer = TimeoutTimer.StartMillisecondsTimeout(nextTimeoutInterval); | ||
| TimeoutTimer intervalTimer = TimeoutTimer.StartNew(TimeSpan.FromMilliseconds(nextTimeoutInterval)); | ||
|
|
||
| // Re-allocate parser each time to make sure state is known. If parser was created | ||
| // by previous attempt, dispose it to properly close the socket, if created. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main changes here are adding support for TimeProvider so that we can fake time in tests and adding a CancellationTokenSource generator method.
This class is also refactored for API clarity. Force instance creation through StartNew to make clear that the countdown starts from the moment of initialization. Prefer TimeSpan over numeric types with implicit units. Consolidate "Start" methods to one that accepts TimeSpan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comprehensive test coverage to verify this class's behavior.