Issue Details (XML | Word | Printable)

Key: DNET-852
Type: Bug Bug
Status: Closed Closed
Resolution: Won't Fix
Priority: Blocker Blocker
Assignee: Jiri Cincura
Reporter: Dominik Psenner
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
.NET Data provider

Ado net provider causes firebird server to leak handles until it is unable to process any requests any longer

Created: 20/Sep/18 01:22 PM   Updated: 05/Oct/18 09:06 PM
Component/s: ADO.NET Provider, Entity Framework
Affects Version/s: 5.12.1.0, 6.0.0.0, 6.1.0.0, 6.2.0.0, 6.3.0.0
Fix Version/s: None

File Attachments: 1. Zip Archive PseudoApplication.zip (10 kB)

Environment: Windows 7, Windows Server 2016, Firebird 2.5.3, Firebird 2.5.4, Firebird 2.5.8, Firebird superserver, Firebird superclassic, Firebird classic
Issue Links:
Duplicate
 
Relate
 


 Description  « Hide
After an update of the firebird ado.net driver from 5.12.0 to 5.12.1 or 6.3 we observe that the firebird server 2.5.x leaks memory that is related to a connection and/or the statements that a client runs against the firebird database using the ado.net provider > 5.12.0.

We observe both an increase in the private memory set (4GB) but also the nonpaged pool increases significantly (5000K) in the firebird server process until the firebird server process is no longer able to handle any statements. At this point the server returns IscError 335544761 "too many open handles to database" and the SQLSTATE is "HY000". This causes the offending client to crash.

As soon as the connection closes, the firebird server is able to recover. It does then free the allocated memory and the nonpaged pool decreases back to normal (<160K).

This may relates to changes implemented with in release 5.12.1. Before ado.net provider 5.12.1, the connection pool to the firebird server did not work properly and connections were closed too early. Connections used to be always short-living. 5.12.1 fixed that and from then on connections are properly pooled. This causes connections to become long-living as long as there is activity. If connections are not closed, the server end will not end up the resources it has allocated and sooner or later show the symptom observed.

We cannot disable the connection pool because of performance reasons. We cannot revert back to 5.12.0 because the connection pool would not work either and we have no further option. Therefore I set the priority of this issue to blocker.

We do also have a minimal sample application that reproduces the issue with a very simplistic database containing only one table with three columns.

 All   Comments   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Dominik Psenner added a comment - 20/Sep/18 01:24 PM
I have attached the reproducing sample application to this issue.

Dominik Psenner added a comment - 20/Sep/18 01:25 PM
Added the firebird versions and flavors affected.

Jiri Cincura added a comment - 04/Oct/18 06:37 AM - edited
OK, the cause of the problem is that EF6 is not explicitly disposing commands. I'm investigating how is the finalizer catching up and how the `ReleasePreparedCommands` fits into the play, because it should take care of it eventually.

Dominik Psenner added a comment - 04/Oct/18 07:23 AM - edited
Please don't implement dispose strategies that rely on the garbage collector. The GC is indeterministic and therefore not the tool to release unmanaged resources like command handles on the firebird server.

I just investigated how the npgsql ado.net provider releases database commands and it seems there is no special handling involved in that implementation. See the EF6 adapter implementation at [1] and [2]. As far as I can tell, everything relies on the invocation of IDisposable.Dispose(). NpgsqlCommand does even request the GC to suppress finalizing the instance because it implements the disposable pattern and frees up the resources by itself, see [3]. NpgsqlConnection does no special handling either and implements dispose and calls GC.SuppressFinalize() too. On dispose it frees up the NpgsqlConnector instance that tracks the current interaction with the npgsql server (see [4]).

[1] https://github.com/npgsql/EntityFramework6.Npgsql/blob/dev/src/EntityFramework6.Npgsql/NpgsqlServices.cs
[2] https://github.com/npgsql/EntityFramework6.Npgsql/blob/dev/src/EntityFramework6.Npgsql/NpgsqlConnectionFactory.cs
[3] https://github.com/npgsql/npgsql/blob/dev/src/Npgsql/NpgsqlCommand.cs
[4] https://github.com/npgsql/npgsql/blob/dev/src/Npgsql/NpgsqlConnection.cs

Jiri Cincura added a comment - 04/Oct/18 07:41 AM

Dominik Psenner added a comment - 04/Oct/18 08:26 AM
Interestingly, when I update the sample application to read as follows, the dispose method on the fbconnection is invoked but the statements are not freed on the server. With this, the owner of the connection is the calling application and the calling application ensures disposal of the connection. Note that the connection string still has pooling enabled. Shouldn't all resources of a connection be freed on disposal, even if it is a pooled one?

                using (var dbConnection = new FbConnection(connectionString))
                using (var dbContext = new PseudoDbContext(dbConnection, false))
                using (var transaction = dbContext.BeginReadWriteTransaction())
                {
                    Console.WriteLine("Opened connection + transaction");
                    int itemsToRead = 1;
                    if (isBurst)
                    {
                        itemsToRead = 100;
                    }

                    for (int i = 0; i < itemsToRead; i++)
                    {
                        ReadItem(random, dbContext);
                    }

                    Console.WriteLine("Disposed connection + transaction");
                }

Jiri Cincura added a comment - 04/Oct/18 08:39 AM
Kind of. When the connection is disposed, alive resources are disposed. But because connection tracks commands via weak references, some commands might have been finalized already. I vaguely remember probably 10 years ago the change to weak references because of some other memory "leak" by something (don't remember what it was anymore), but this was "leaking" on client side.

Thinking about it now, before it was done because in the code there was lot of wrong finalizers, which I've recently cleaned up. So it might actually make sense to use regular references.

I wish I remember clearly what exactly the issue was, to see whether the change is safe. Also I'm not sure how is this going to play out when one FbCommand would be used with multiple connections. I'll have to try that out.

Jiri Cincura added a comment - 04/Oct/18 08:41 AM
Created a ticket for that - DNET-854. Shouldn't take too long to try it out.

Dominik Psenner added a comment - 04/Oct/18 08:52 AM
> connection tracks commands via weak references

I do hope that on the client, handles that reference resources on the server are not weak references. If that's the case the client is allowed to remove instances that reference server side resources to save memory. This in turn surfaces as very interesting symptoms like the server leaking handles and memory. :-)

> Also I'm not sure how is this going to play out when one FbCommand would be used with multiple connections.

In all rdbms db commands are tightly related to server resources. DbConnection offers the CreateCommand() method for a reason. Developers shall fix the code if they happen to find such a usage.

Jiri Cincura added a comment - 04/Oct/18 08:58 AM
> I do hope that on the client, handles that reference resources on the server are not weak references. If that's the case the client is allowed to remove instances that reference server side resources to save memory. This in turn surfaces as very interesting symptoms like the server leaking handles and memory. :-)

Well, as long as you keep the reference somewhere in rooted variable, it doesn't matter because the finalizer will not be able to collect that command. And if you've forgot to dispose it and lost the reference, well, it's your fault anyway.

Dominik Psenner added a comment - 05/Oct/18 06:18 AM
Arthur Vickers closed https://github.com/aspnet/EntityFramework6/issues/639 with won't fix. EF providers shall implement a workaround because EF6 is not going to implement any dispose on instances that implement the disposable pattern. Ideally such a workaround is limited to the EF6 integration only but it will be hard since EF6 does not inform the provider when it does no longer need a command.

Jiri Cincura added a comment - 05/Oct/18 11:08 AM
That's what DNET-854 is about, if it proves to be doable. At the moment you can clear the pool regularly to really disconnect and then server will free the statements.

Dominik Psenner added a comment - 05/Oct/18 11:21 AM
Is there a way to use a custom connection pool?

Jiri Cincura added a comment - 05/Oct/18 11:28 AM
What is a custom connection pool?

Dominik Psenner added a comment - 05/Oct/18 11:40 AM
The current fb connection pool implementation keeps connections open with a configurable min lifetime. I thought of force disposing connections after a configurable maximum lifetime. For instance, connections could be kept alive for a minimum of 15 seconds as long as the connection handles commands but is closed after at most 5 minutes to recycle any leaked handles.

Jiri Cincura added a comment - 05/Oct/18 11:49 AM
You can't do that directly, but you can use FbConnection.ClearPool after max lifetime was exceeded. Different pools you can get by using different values in connection string, for example `ClientLibrary` can be used as dummy value that's changing.

Dominik Psenner added a comment - 05/Oct/18 02:59 PM
After having a conversation in the team, we chose to stick with 5.12.0 without implementing any hacks or workarounds. The reasoning is that the software is going to behave like it did until now and we already learned how to handle that. This also means to us that we will have to re-evaluate our options in the near future. We do hope that until then the dotnet firebird provider implements a workaround such that it does no longer cause the firebird server to leak handles when used with EF6. We do further hope that the connection pool is improved such that it recycles connections that exceed a configurable maximum lifetime. Thanks for your work and your valuable insights. Cheers!

Jiri Cincura added a comment - 05/Oct/18 03:16 PM
Maximum lifetime is surely not going to be implemented. It's not something that's "standard" in ADO.NET.

Dominik Psenner added a comment - 05/Oct/18 05:21 PM
The reasoning behind such a configuration is that, while not standard among ado.net providers, it is an effective workaround against all kind of resource leaks that are disposed when a connection is closed.

Would you accept a contributed pull request on github?

Jiri Cincura added a comment - 05/Oct/18 09:06 PM
I would very likely reject that.