Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FbConnectionPoolManager does not remove disconnected FbConnectionInternal instances [DNET678] #631

Closed
firebird-issue-importer opened this issue Apr 5, 2016 · 5 comments

Comments

@firebird-issue-importer
Copy link

@firebird-issue-importer firebird-issue-importer commented Apr 5, 2016

Submitted by: Hans Torm (hfztt)

Is related to DNET668

Votes: 1

If a FbConnection's FbConnectionInternal gets disconnected from the server in the underlying System.Net.Sockets.NetworkStream, this is not detected by the FbConnectionInternal instance. Thus the FbConnectionInternal instance will keep on being released to the FbConnectionPoolManager upon Close and reused for later connections, creating errors.

The problem is compounded by the fact that even if you detect the Error, you cannot simply Dispose the FbConnection as this will still release the FbConnectionInternal instance to the Pool. Only workaround is to flush the Pool, but to do that you have to break the encapsulation provided by the http://ADO.NET abstractions and link directly to the FbDriver to get access to the Pool management features on FbConnection, thus preventing DB agnostic code. This is not desirable.

Alternatively not disposing a failed connection will work as the FbConnectionInternal instance will never be GC'd due to being in the _busy List, but will of course introduce a subtle memory leak. This is not desirable.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Aug 21, 2016

Modified by: @cincuranet

Link: This issue is related to DNET668 [ DNET668 ]

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Nov 15, 2016

Commented by: Jiri Fartak (jfartak_wms.cz)

Hi Jiri,

I didn't study the whole architecture of .Net provider, however what about of the idea of adding connection status property like "IsOpen" to the FbConnectionInternal whose getter would propagate this call to underlying IDatabase's socket? I think that indirect testing of underlying Socket's status via polling would do the needed behavior.

The FbConnectionPoolManager is the resource owner/factory and thus it should provide valid resource when someone asks for it. Further, it can destroy the broken connections on these occasions:

1) When the FBConnectionInternal is returned to the pool (after FBConnection.Dispose() method call)- if the test (IsOpen call) would fail, then FbConnectionINternal is destroyed immediately and not included into the _available sub-pool.

2) When the FbConnectionPoolManager is asked for acquring a resource - existing or new connection - then after any available connection is popped up from the _available stack, then if IsOpen call would fail, then such connection is destroyed immediately and this would repeat until any opened connection is found. If so, then it is returned. This process would also naturally clean up the _available sub-pool. If no such connection is found (_available.Count reaches zero), then new one is created. By other words acquiring method (in pseudo code) would look like:

public FbConnectionInternal GetConnection(FbConnection owner) would do the thing:
{
lock (_syncRoot)
{
CheckDisposedImpl();

				FbConnectionInternal connection = null;
				while\(\_available\.Any\(\)\)
				\{		
					//this loop naturally cleans up broken connections

					connection = \_available\.Pop\(\)\.Connection;
					if \(\!connection\.IsOpen\)    //test the connection state \(polling\)
						\{
							Destroy\(connection\); \-\-\- this would immediately destroys\(GC\) the connection
						\}
				\}
			        if \(connection == null\)
					connection = CreateNewConnectionIfPossibleImpl\(\_connectionString, owner\);	//the pool is empty \(either all were all destroyed since they were broken or nothing was available\), let's try to create new one
				connection\.SetOwningConnection\(owner\);
				\_busy\.Add\(connection\);
				return connection;
			\}
		\}

3) GC (when its time comes) can pro-actively polls the FBConnectionInternal.IsOpen flag and if false is returned then such broken connections will be destroyed and collected.

The fact, that broken connections can live for some time in the _available sub-pool, is not problem. Yes, they will stay there either until GC thread will collect them or when acquiring them for next use.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 21, 2017

Modified by: @cincuranet

status: Open [ 1 ] => In Progress [ 3 ]

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 21, 2017

Commented by: @cincuranet

The problem is that there's no way to know whether the socket is connected. The Connected reflects the state of the connection as of the most recent operation. So there would have to be dummy operation (making everything slower). And even that. When the connection is returned from pool because the IsOpen was true, there's no guarantee it's still opened at the point of returning it to developer. That's why the DNET668 was introduced.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 21, 2017

Modified by: @cincuranet

status: In Progress [ 3 ] => Closed [ 6 ]

resolution: Won't Fix [ 2 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants