Sap connections not being closed.

[zoki986]

Hi, I'm using version 1.0.0.62 of SapNwRfc and have an API that is calling SAP RFC functions, and everything works fine until SAP becomes unavailable.
When SAP is not available or it takes too long to establish a connection I get this exception.

System.NullReferenceException: Object reference not set to an instance of an object.
at SapNwRfc.Pooling.SapConnectionPool.ForgetConnection(ISapConnection connection)
at SapNwRfc.Pooling.SapPooledConnection.InvokeFunction[TOutput](String name, Object input, CancellationToken cancellationToken)

The connection limit is reached ( it was 5 default value) and SapConnectionPool.GetConnection returns null which afterward produces the above exception.

I have temporarily resolved this by increasing pool size and max pool size and app pool recycle after 1 hour.

Is there some way that this can be handled without increasing pool size to a large number?

[huysentruitw]

Please try again with version 1.0.0.84, I think this bug was already addressed a few months ago.

[huysentruitw]

Closing this issue as the new version should fix this issue (we had the same problem in the past). Feel free to re-open if after updating the problem persists.

[Snejki]

Hi, unfortunatelly problem stiil encounters on 1.2.0 version

[huysentruitw]

I no longer have access to an SAP server, so if you can, please help debug this issue. Normally, when connections are broken, you should arrive in the catch part here where the connection is returned to the pool. If that doesn't happen, then that will be the reason of the pool starvation.

[Snejki]

Well, actually it is the case, when connection with Sap is not established, so you do not need access to SAP to recreate this problem.

I believe there is some problem when SapConnection object can not establish connection with Sap when Calling Connect() method of this object. Disposing this object causes Exception which is mentioned in the first post of this thread. This causes problems in SapConnectionPool ForgetConnection() method which is called from the part of code you mentioned (from catch block).

/// <inheritdoc cref="ISapConnectionPool"/>
        public void ForgetConnection(ISapConnection connection)
        {
            connection.Dispose();
            lock (_syncRoot) _openConnectionCount--;
            _idleConnectionSemaphore.Release();
        }

As we can see the first line of this method tries to Dispose SapConnection object (object with not established connection) which in this case throws exception and next lines of methods are never called (_openConnectionCount is not decreased), so when we call this method _poolSize times, the next connections in GetConnection() will never be created

[huysentruitw]

I have a fix for this ready. I only have one question: if connect fails during a call to InvokeFunction does it make sense to try to connect for a second time directly before throwing an exception?

The reason why I'm asking is because there will be no delay between the two connection attempts, so the chance a connection will be made the second time is probably very small... and not worth the retry. What do you think @Snejki or @zoki986 ?

cc @campersau @jwienekamp

[campersau]

@huysentruitw That is what I had done here #53 (comment) but reverted it because of your comments #53 (comment).
One of the InvokeFunction methods already did the

_connection = _connection ?? _pool.GetConnection(cancellationToken);

outside of the try catch and even has a test specific for this behavior.

SapNwRfc/src/SapNwRfc/Pooling/SapPooledConnection.cs

Lines 52 to 71 in 1811294

	public void InvokeFunction(string name, CancellationToken cancellationToken = default)
	{
	_connection = _connection ?? _pool.GetConnection(cancellationToken);
	try
	{
	using (ISapFunction function = _connection.CreateFunction(name))
	function.Invoke();
	}
	catch (SapCommunicationFailedException)
	{
	// Let the pool collect the dead connection
	_pool.ForgetConnection(_connection);
	// Retry invocation with new connection from the pool
	_connection = _pool.GetConnection(cancellationToken);
	using (ISapFunction function = _connection.CreateFunction(name))
	function.Invoke();
	}
	}

---

1. If we don't move all the

_connection = _connection ?? _pool.GetConnection(cancellationToken);

code out of the try catch then an addition null check is needed before ForgetConnection.

In addition to that the _openConnectionCount in the SapConnectionPool should only be increased after a successful Connect call.

Here is a fix for this case: campersau@2776646

---

2. Fix for the other case when we move all the

_connection = _connection ?? _pool.GetConnection(cancellationToken);

code out of the try catch: campersau@0d4351b

---

I prefer option 2.

[huysentruitw]

Package version 1.4.0 has been published that should fix this issue. I'll close this issue for now.