System.Data.SQLite
View Ticket
Not logged in
Ticket UUID: 0e48e80333679f92060d16d33005ea12c2d6cffe
Title: Unclear interaction between connection event handlers and connection pooling vs. MDA errors
Status: Closed Type: Code_Defect
Severity: Important Priority: Medium
Subsystem: Integration_Via_PInvoke Resolution: Fixed
Last Modified: 2017-11-14 16:20:19
Version Found In: 1.0.105.2
User Comments:
anonymous added on 2017-11-08 17:49:23:
It's unclear what the expected behaviour(s) are when using callbacks on the connection in combination with connection pooling

It's very easy to trigger the callbackOnCollectedDelegate MDA (https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/callbackoncollecteddelegate-mda) even with handlers that should not ever be GC'ed 

(so I'm guessing the handlers being collected are probably inside the library itself, when the connection is reused, rather than in user level code)

I provide example code here:
https://gist.github.com/DavidR91/478ab0894b2795fbbb2403508a93e3ab#file-program-cs

Wait 2-3 seconds and an MDA error will occur.

Note how the handler is an explicitly instantiated static delegate (so as far as I understand, it should not be GC'ed)

If you disable pooling, everything works fine - because the connection is disposed and whether event handlers are attached or detached is moot

---------------------

The issue comes down to this:

* Is it expected that handlers will malfunction in this way when using connection pooling if they are not explicitly detached?

* Is it expected that detaching handlers during the Disposed event does not work to prevent this problem? (I'm anticipating yes, because Disposed is surfaced from the Component object and not this library itself - but having some explicit answer/documentation on whether and when to use this event will be helpful)

mistachkin added on 2017-11-09 06:31:03:
Thanks for the detailed report.  I think the only realistic "solution" to this,
given the use of weak references by the connection pool, is to either prevent a
connection with event handlers from being added to the pool upon being closed or
remove (or clear) the handlers prior to adding it to the pool.

mistachkin added on 2017-11-09 08:06:00:
I've checked in changes that should address this issue.  Is it possible for you
to test them in your environment and report back if it solves the issue you are
seeing?

Meanwhile, I'll study your example and add test cases here.

mistachkin added on 2017-11-09 08:08:57:
The changes are on the "tkt-0e48e80333" branch, here:

[/info/tkt-0e48e80333]

You can update your Fossil checkout, if any, using:

fossil update tkt-0e48e80333

Alternatively, a snapshot of the checkout can be downloaded as a ZIP
archive from the link above.

mistachkin added on 2017-11-09 08:32:10:
Also, for the Disposed event handler, it will be called *after* the connection
and all of its resources have been disposed.  So, it cannot make use of any of
those resources, including modifying any other event handlers.

anonymous added on 2017-11-09 10:27:56:
Can confirm the fix works for my test code here - I'm just working to try and predictably recreate the issue in our product so I can do before vs. after comparison for something more real-world (quite a bit harder to get it to predictably occur for whatever reason though)

+ The disposed event stuff makes sense, and I started to get that impression while messing with this issue, so no shocker there, understood.

Thank you for the quick turnaround on the change by the way - I'll get back ASAP with what I find in our product, but I'm pretty hopeful this sorts out the problem

anonymous added on 2017-11-09 11:19:14:
I produced the MDA issue with the *old* DLL in our product, and since replacing it with the patched version, I haven't been able to recreate this issue

So between both the test code and our product, I'm confident that this change fixes the problem.

Note though that although the product exercises all of the event handlers (commit, rollback, update etc.) I've only ever seen the MDA fire for the trace event (presumably because it's way more frequent) - so it will be difficult to say with confidence whether this issue is fixed for all the event types - but I guess that's where test cases come in

mistachkin added on 2017-11-14 16:20:19:
Fixed on trunk, with tests.