search

Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
356 stars 62 forks source link

Fixed threading issues with managing connections #18

Closed SartorialOffense closed 2 years ago

SartorialOffense commented 2 years ago

Hi thanks for creating this project! I did see a threading issue with the connection caching and fixed it: pushed all the logic for connections into a ConnectionManager class, added/renamed some of the internal stuff and put into Internal folder so the main classes didn't get crowded. Added testing project to verify the bug was fixed. Let me know if you need me to change anything.

Giorgi commented 2 years ago

@SartorialOffense Thanks for taking the time to send a PR!

Is it possible to implement it with ConcurrentDictionary and avoid locking the dictionary?

SartorialOffense commented 2 years ago

I tried to do that initially by keeping the concurrent dictionary and locking on each database file reference while being changed, but I kept running into scenarios where bad things would happen. I figured out a way to do it (I think) today. Let me play with it a couple days this week. The trick is that on disposal/removal of the file reference, you need to anticipate the case where while that is happening, another thread might have just taken a lock on the same file reference object. So you have to put it back in the dictionary and undispose once you acquire the lock.

It does make bugs more likely because you can't simply use lock() on each file reference. You have to use Monitor.Enter() and Exit() and handle any errors.

On Sun, Nov 14, 2021 at 1:23 PM Giorgi Dalakishvili < @.***> wrote:

@SartorialOffense https://github.com/SartorialOffense Thanks for taking the time to send a PR!

Is it possible to implement it with ConcurrentDictionary and avoid locking the dictionary?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Giorgi/DuckDB.NET/pull/18#issuecomment-968348894, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2Z5OSTSV6HJONVXB24ZWDUMAEBHANCNFSM5H7EBNAQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Giorgi commented 2 years ago

I'll accept the PR as is and if you make changes you can send a new one.

SartorialOffense commented 2 years ago

Great, I just put one in. Seems to work pretty well and is indeed a couple times faster under heavy multithreaded load.

On Mon, Nov 15, 2021 at 10:04 AM Giorgi Dalakishvili < @.***> wrote:

I'll accept the PR as is and if you make changes you can send a new one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Giorgi/DuckDB.NET/pull/18#issuecomment-969059928, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2Z5OSLYHKQYFE3IBJMP6DUMEVRJANCNFSM5H7EBNAQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.