MaxConnLifetime #223
MaxConnLifetime #223
Conversation
| _maxConnLifeTime = maxConnLifeTime; | ||
| } | ||
|
|
||
| public bool IsConnectionReusable(IPooledConnection connection) |
There was a problem hiding this comment.
Shall we change this method's name to be more self describing such as ResetAndRestartIdleTimer because it has side effects aside from checks.
There was a problem hiding this comment.
My thinking was that the outside of this object should not care about Idle timer or life time. The contract was that once this method returns true, then this connection can be reused, a.k.a. putting back to the connection pool. So I would prefer the name stays this IsConnectionReusable
|
|
||
| try | ||
| { | ||
| connection.ClearConnection(); |
There was a problem hiding this comment.
Not specific for this PR, but if in the middle of a huge resultset streaming, calling RESET may stop the server from streaming more data but it will not help client's consuming pending network buffers - which may take considerable time. Shall we have a means of timeout-ing?
There was a problem hiding this comment.
Right now we do not provide any API method to stop streaming. Internally our current sessoin.close will also not stop any streaming but buffering all unconsumed results into memory. So we do not need to worry about timing out now.
But this is interesting when we want to add cancellation token into our async methods.
| return true; | ||
| } | ||
|
|
||
| public async Task<bool> IsConnectionReusableAsync(IPooledConnection connection) |
There was a problem hiding this comment.
The same two comments applies for this method.
| var connectionSettings = new ConnectionSettings(uri, AuthTokens.None, Config.DefaultConfig); | ||
| var poolSettings = new ConnectionPoolSettings(1, 1, TimeSpan.MaxValue, TimeSpan.MaxValue); | ||
| var connectionSettings = new ConnectionSettings(AuthTokens.None, Config.DefaultConfig); | ||
| var poolSettings = new ConnectionPoolSettings(1, 1, Config.Infinite, Config.Infinite, Config.Infinite); |
There was a problem hiding this comment.
We may elaborate on what TimeSpan value makes it Infinite. I see the point that we accept -1 as an Infinite, but since we define the type of setting timeout values as TimeSpans, any TimeSpan value less than zero, zero itself or even TimeSpan.MaxValue could be perceived as infinite values. Not a big overhead when set something different than -1, but we shall be more natural.
There was a problem hiding this comment.
Found System.Threading.TimeOut static class which declares InfiniteTimeSpan static field which is also using -1. No change needed then, but we may point out this in the docs.
There was a problem hiding this comment.
The current decision is that we treat all negative values as Infinite, which is stated in the API docs of Config for these TimeSpan settings.
The Config.Infinite value is internal and as you figured out I get this number from some standard lib.
zhenlineo
force-pushed
the
1.5-conn-pool
branch
from
b0e429a
to
7eed3c6
Compare
| { | ||
| if (IsConnectionIdleDetectionEnabled()) | ||
| { | ||
| connection.IdleTimer.Start(); |
There was a problem hiding this comment.
I think it is a bit strange that validator resets timers. Can this happen in the connection pool instead?
Same happens in HasBeenIdleForTooLong.
| /// <param name="timeSpan">The max timespan that a connection can be reused after has been created for.</param> | ||
| /// <returns>An <see cref="IConfigBuilder"/> instance for further configuration options.</returns> | ||
| /// <remarks>Must call <see cref="ToConfig"/> to generate a <see cref="Config"/> instance.</remarks> | ||
| IConfigBuilder WithMaxConnectionLifeTime(TimeSpan timeSpan); |
There was a problem hiding this comment.
Should publicly visible name be Lifetime and not LifeTime?
zhenlineo
force-pushed
the
1.5-conn-pool
branch
from
ba7bceb
to
d171271
Compare
Added maxConnectionLifeTime into config.
When this config option is set to a non-negative value, the connection will be closed rather than reused when it is obtained from the connection pool.