tomasfabian / Joker

Reactive data changes from SQL server to .NET clients. SqlTableDependency extensions, Joker.OData, Joker.Redis, Joker.MVVM and ksqlDB LINQ provider
MIT License
69 stars 23 forks source link

Possible issue with Dispose in SqlTableDependencyWithReconnection #11

Closed lekrus closed 3 years ago

lekrus commented 3 years ago

Hello,

We are trying to use your extension to solve the issue with bad triggers / deadlock with original SqlTableDependency component. https://github.com/christiandelbianco/monitor-table-change-with-sqltabledependency/issues/188

And after preliminary testing we see it's working just great. The only issue we have is an exception that is thrown in Dispose method that is currently implemented like:


protected override void Dispose(bool disposing)
{
    if (LifetimeScope != LifetimeScope.UniqueScope)
      DropDatabaseObjects();

    base.Dispose(disposing);
}

for main Dispose call it works fine, but when a container disposes objects, the following is called:

~TableDependency() => this.Dispose(false);

and we get "Handle is not initialized" from System.Data.ProviderBase.DbConnectionInternal.PostPop exception, most probably because parent object is already disposed.

So I think the proper implementation of that Dispose should include disposing check:


protected override void Dispose(bool disposing)
{
    if (disposing && LifetimeScope != LifetimeScope.UniqueScope)
      DropDatabaseObjects();

    base.Dispose(disposing);
}

Could you please check the issue?

Thank you!

tomasfabian commented 3 years ago

Hi @lekrus, thx for your observation, I would like to add yet another check inside the above mentioned Dispose fix. Do you agree with this?

    protected override void Dispose(bool disposing)
    {
      if (_disposed)
        return;

      if (disposing && LifetimeScope != LifetimeScope.UniqueScope)
        DropDatabaseObjects();

      base.Dispose(disposing);
    }
tomasfabian commented 3 years ago

@lekrus I wanted to test the above mentioned code, but the finalizer is not called in .NET 5 linux container. I tried this in a separate project:

  public class Test
  {    
    ~Test()
    { 
      Console.WriteLine("Finalizing a Test");
    }
  }

  public class Program
  {
    public static void Main(string[] args)
    {
      var test = new Test();

      test = null;

      //Force garbage collection.
      GC.Collect();

      GC.WaitForPendingFinalizers();

      CreateHostBuilder(args).Build().Run();
    }
  }

Do you know how could I force to call the finalizer in order to test it?

lekrus commented 3 years ago

Hello, Thanks for a quick reply. Not sure re adding that clause, as _disposed is a variable managed by a base class, so if we later direct to base.Dispose it should be handled there?

On a side note, while checking where _disposed is used in your overriding class I noticed that Stop method override can be removed at all as base.Stop and your "else" clauses do the same?

public override void Stop()
    {
      if (this.LifetimeScope == LifetimeScope.ConnectionScope)
      {
        base.Stop();
      }
      else
      {
        if (this._task != null)
        {
          this._cancellationTokenSource.Cancel(true);
          this._task?.Wait();
        }
        this._task = (Task) null;
        this._disposed = true;
        this.WriteTraceMessage(TraceLevel.Info, "Stopped waiting for notification.");
      }
    }

or that was just planned for some future updates to the logic?

tomasfabian commented 3 years ago

@lekrus thats true, re-adding the _disposed check won't help. I'm usually "paranoic" due to data races, but _disposed is not thread safe, so it does not matter in this case at all. It will be set at the end of the base.Dispose...

Regarding the Stop you are right again, it is not necessary to override it. It is probably a "fossil" as I use to call code which is not needed any more, but remains for some reasons in the code base. :D I will check the history and "blame" myself in Git.

I'm not sure what happens in your case (see my previous post) when the Dispose is called with disposing == false. Were the database objects dropped during regular disposal (disposing == true)?

tomasfabian commented 3 years ago

@lekrus isn't it an issue for you that the database objects are not dropped during the finalization?

lekrus commented 3 years ago

Hello Tomas, sorry, didn't have time yet to provide more detailed sample. We are using https://github.com/Topshelf/Topshelf to create a service and then when Ctrl-C is pressed and the service is being terminated, disposal of all objects is performed. When doing debug, Dispose is called both with true and false and the latter call is causing the issue, as some internal DB connection objects are already not available.

In any case, Dispose(true) properly cleans all DB objects, so even with that exception with Dispose(false) everything works correctly, but it's again an exception, that's why I decided to submit this issue.

tomasfabian commented 3 years ago

I released v2.3.4. Thank you @lekrus for your help

lekrus commented 3 years ago

thank you for a quick release, it's working just fine now