Dnn.Platform icon indicating copy to clipboard operation
Dnn.Platform copied to clipboard

DataProvider and Dependency Injection SQL Connection Pool Limits

Open SkyeHoefling opened this issue 3 years ago • 7 comments

Description of bug

The DataProvider is starting to be used in objects that are being registered in the Dependency Injection container. Transient registered objects that use the DataProvider will create a new database connection every time the DataProvider.Instance() is executed. If an instance is requested many times in a single call this could spam the database with connection requests. While working on a change I noticed the connection pool limit would hit capacity and DNN will throw an exception and stop functioning. To solve this problem we will need to come up with a strategy for the DataProvider to be registered with Dependency Injection. The implications of this are massive since so much of the platform depend on the DataProvider.

Code Example

The code sample below should help explain the problem

public interface IMyService
{
    void DoSomething();
}

public class MyService : IMyService
{
    public void DoSomething()
    {
        var provider = DataProvider.Instance();
        // omitted code
    }
}

Now suppose this service is being used in multiple places at once. The best example I have is the Globals static apis are now wrapping Dependency Injection calls. Consider the following code

public static class Globals
{
    // omitted code

    public static void DoSomething() => DependencyProvider.GetRequiredService<IMyService>().DoSomething();

    // omitted code
}

If there is a call stack that invokes Globals.DoSomething() multiple times, it will resolve a new instance of IMyService every single time which will create a separate SQL connection from the DataProvider.Instance() statement.

Steps to reproduce

see description above

Current behavior

SQL Connection Pool Limit can be exceeded with dependencies that use DataProvider.Instance()

Expected behavior

The DataProvider should be registered with the Dependency Injection container to limit the number of instances that can be created at once

Screenshots

N/A

Error information

I don't have the error log anymore, as I identified this problem while debugging a platform change I was working on

Additional context

N/A

Affected version

  • [x] 10.00.00 alpha build
  • [x] 09.07.00 release candidate
  • [x] 09.06.02 latest supported release

Affected browser

N/A

SkyeHoefling avatar Aug 16 '20 19:08 SkyeHoefling

cc: @mitchelsellers @bdukes

SkyeHoefling avatar Aug 16 '20 19:08 SkyeHoefling

Interesting, This could be the time where rolling our own singleton pattern could be a bit tricky

mitchelsellers avatar Aug 16 '20 19:08 mitchelsellers

The DataProvider.Instance() method uses the Component Factory. I created a new work item for updating the component factory to use Dependency Injection #3955 . If we get that done we could register an IDataProvider as a singleton which should alleviate this problem and give us some database performance improvements. There could be downsides of registering it as a Singleton but it would be a start.

SkyeHoefling avatar Aug 16 '20 22:08 SkyeHoefling

We have detected this issue has not had any activity during the last 90 days. That could mean this issue is no longer relevant and/or nobody has found the necessary time to address the issue. We are trying to keep the list of open issues limited to those issues that are relevant to the majority and to close the ones that have become 'stale' (inactive). If no further activity is detected within the next 14 days, the issue will be closed automatically. If new comments are are posted and/or a solution (pull request) is submitted for review that references this issue, the issue will not be closed. Closed issues can be reopened at any time in the future. Please remember those participating in this open source project are volunteers trying to help others and creating a better DNN Platform for all. Thank you for your continued involvement and contributions!

stale[bot] avatar Nov 24 '20 23:11 stale[bot]

This issue has been closed automatically due to inactivity (as mentioned 14 days ago). Feel free to re-open the issue if you believe it is still relevant.

stale[bot] avatar Dec 09 '20 09:12 stale[bot]

@valadas this is another issue that really needs to be marked as pinned. It will become more and more of a problem as more parts of DNN support Dependency Injection

SkyeHoefling avatar Dec 11 '20 00:12 SkyeHoefling

I totally missed this mention @ahoefling just had a need for this and ended up here to see the mention :)

valadas avatar Jan 20 '22 05:01 valadas