-
Notifications
You must be signed in to change notification settings - Fork 299
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
PERF | Cleanup IDisposable in SqlDataReader #921
Conversation
Anywhere that the disposable object is being passed into the AAsyncCallContext class you cannot use a using block. The async context keeps the registration alive for the duration of the call and because it's an async call that object lives longer than the method that the object is created in. The disposable registration is correctly disposed of either in the end or clear methods. If you find others non-disposed in disposable methods i'd be careful to check if they're part of async invocations. |
In netcore NextResultAsync there is a check to see if there is a real cancellation token (excludes CancellationToken.None which can't be cancelled) and then there is the call to register a delegate to be invokes if cancellation is requirest by the user:
private sealed class HasNextResultAsyncCallContext : AAsyncCallContext<bool>
{
private static readonly Func<Task, object, Task<bool>> s_execute = SqlDataReader.NextResultAsyncExecute;
internal override Func<Task, object, Task<bool>> Execute => s_execute;
} which means we'll end up calling back into the SqlDataReader NextResultAsyncExecute function which does some custom logic to check if there is more data and then calls into ExecuteAsyncCall which goes through a confusing set of callbacks and methods to do with early exiting and avoiding more waits but ultimately ends up through all routes at CompleteAsyncCall. private void CompleteAsyncCall<T>(Task<T> task, AAsyncCallContext<T> context)
{
TaskCompletionSource<T> source = context._source;
context.Dispose(); The dispose call for internal void Clear()
{
_source = null;
_reader = null;
IDisposable copyDisposable = _disposable;
_disposable = null;
copyDisposable?.Dispose();
} this clears the fields in case the object will be reused and then disposes the registration we started with earlier if it has been set. In this case the object isn't reused but for other more common async methods like public override void Dispose()
{
SqlDataReader reader = this._reader;
base.Dispose();
reader.SetCachedReadAsyncCallContext(this);
} So as you can see it takes a copy of the reader variable which is about to be nulled, calls the base Dispose() which will call Clear() and then returns the object to the reader so it can use it for the next call. The netfx code does this same logic but using anonymous delegates and closures which i find a lot harder to read through. They're functionally equivalent. |
Ok. I will close this PR, but will do some further testing in future on this case. Thanks @Wraith2 for continuous support. |
There are several places in SqlDataReader that object of type IDisposable has been created but never disposed.
This PR wraps them all in
using
block to make sure all those objects have been disposed properly.The
AAsyncCallContext
class:I believe we need to dispose the original object,
_disposable
, and set the same object to null after that, but I need to do some testing on that part before doing that to avoid any unseen situations. For now, setting it to null is moved to the line after disposing the copy item to prevent disposing a null object.