You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by FlorianHockmann <gi...@git.apache.org> on 2018/06/23 10:33:40 UTC
[GitHub] tinkerpop pull request #885: Add check for connection state before returning...
GitHub user FlorianHockmann opened a pull request:
https://github.com/apache/tinkerpop/pull/885
Add check for connection state before returning it TINKERPOP-1978
https://issues.apache.org/jira/browse/TINKERPOP-1978
This PR simply adds the requested check of whether a connection is actually open that was returned from the pool before returning it to the driver. Before it was possible that a connection would be used that was already closed while it was in the connection pool.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1978
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/885.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #885
----
commit c4a254726167d798617331ff3dd00ece97377b2d
Author: Florian Hockmann <fh...@...>
Date: 2018-06-23T10:24:15Z
Add check for connection state before returning it TINKERPOP-1978
----
---
[GitHub] tinkerpop pull request #885: TINKERPOP-1978 Gremlin.Net: Add check for conne...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/885#discussion_r197851424
--- Diff: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs ---
@@ -45,19 +44,28 @@ public ConnectionPool(ConnectionFactory connectionFactory)
public async Task<IConnection> GetAvailableConnectionAsync()
{
- Connection connection = null;
- lock (_connectionsLock)
- {
- if (!_connections.IsEmpty)
- _connections.TryTake(out connection);
- }
-
- if (connection == null)
+ if (!TryGetConnectionFromPool(out var connection))
connection = await CreateNewConnectionAsync().ConfigureAwait(false);
return new ProxyConnection(connection, AddConnectionIfOpen);
}
+ private bool TryGetConnectionFromPool(out Connection connection)
+ {
+ while (true)
+ {
+ connection = null;
+ lock (_connectionsLock)
+ {
+ if (_connections.IsEmpty) return false;
+ _connections.TryTake(out connection);
+ }
+
+ if (connection.IsOpen) return true;
+ connection.Dispose();
--- End diff --
Won't that lead to a never-ending loop? I'm assuming that `connection.Dispose()` won't remove it from the `_connections` collection, but I might be wrong.
---
[GitHub] tinkerpop issue #885: TINKERPOP-1978 Gremlin.Net: Add check for connection s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/885
VOTE +1
---
[GitHub] tinkerpop pull request #885: TINKERPOP-1978 Gremlin.Net: Add check for conne...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/885
---
[GitHub] tinkerpop pull request #885: TINKERPOP-1978 Gremlin.Net: Add check for conne...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/885#discussion_r197855449
--- Diff: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs ---
@@ -45,19 +44,28 @@ public ConnectionPool(ConnectionFactory connectionFactory)
public async Task<IConnection> GetAvailableConnectionAsync()
{
- Connection connection = null;
- lock (_connectionsLock)
- {
- if (!_connections.IsEmpty)
- _connections.TryTake(out connection);
- }
-
- if (connection == null)
+ if (!TryGetConnectionFromPool(out var connection))
connection = await CreateNewConnectionAsync().ConfigureAwait(false);
return new ProxyConnection(connection, AddConnectionIfOpen);
}
+ private bool TryGetConnectionFromPool(out Connection connection)
+ {
+ while (true)
+ {
+ connection = null;
+ lock (_connectionsLock)
+ {
+ if (_connections.IsEmpty) return false;
+ _connections.TryTake(out connection);
+ }
+
+ if (connection.IsOpen) return true;
+ connection.Dispose();
--- End diff --
`_connections.TryTake(out connection)` removes the connection from the collection. So, here it is already no longer part of `_connections`. That may be a bit confusing as `_connections` doesn't contain all connections in the pool but only those that are currently unused. That made this class a lot easier to implement as no checks are needed of whether a connection is already in use or not.
For reference: The [`ConcurrentBag<T>.TryTake`](https://msdn.microsoft.com/en-us/library/dd381774(v=vs.110).aspx) docs.
---
[GitHub] tinkerpop issue #885: TINKERPOP-1978 Gremlin.Net: Add check for connection s...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/885
VOTE: +1
---