[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166503#comment-16166503
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
I just cherry-picked the two commits from @jorgebay's PR and merged tp32 
into master afterwards. So master should be in a good state now.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
> Fix For: 3.2.7, 3.3.1
>
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166463#comment-16166463
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
yeah - i remember that now! i wonder how i messed that up.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166439#comment-16166439
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
It's weird because 6a3fc39a912bec98707dd69d461955330627df10 was merged by 
@spmallette on master in 75502eef222b0dfc7aa83c2456ab319a9f9dd1a6 (he beat me 
to it because I was taking too long to do it) and this branch started in 
98e4e1b8201f50e1f34458a82c20f2737aa6ae53, which is down the line.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166400#comment-16166400
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
what the heck?! I wonder what got messed up there. i don't see a merge 
from jorge from tp32 to master after that, so i guess it never got in there, 
yet how could it have been ignored? we did tons of merges from tp32 to master 
after his merge to tp32.

i guess you should cherry-pick them into master and then do your merge. 
that's really strange. i'm not sure what to make of that.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166366#comment-16166366
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
Thanks for the explanation @spmallette. I was about to ask about the 
approach for merging pull requests as the documentation only talks about the 
case where separate pull requests are used for the different release branches. 
(Maybe we should extend that for simpler cases where only a pull request to 
tp32 is enough?)

I just tried to merge tp32 to master, but ran into a merge conflict as 
apparently the changes from TINKERPOP-1744 (PR #690) are missing in master 
although GitHub shows the merge commit also for the master branch: 
https://github.com/apache/tinkerpop/commit/6a3fc39a912bec98707dd69d461955330627df10

Do I now have to cherry-pick them to get them into master before I merge 
tp32 or what is the best approach in cases like this?

I'll close the JIRA ticket as soon as the changes are also in master.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166122#comment-16166122
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
@FlorianHockmann  When you merge a PR to tp32, please make sure you also 
merge tp32 to master. Technically it is best to do that locally first, make 
sure both tp32 and master are building properly, then push master and then 
tp32. 

Also, the JIRA for this PR still needs to closed. Thanks.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16166052#comment-16166052
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user asfgit closed the pull request at:

https://github.com/apache/tinkerpop/pull/704


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16164670#comment-16164670
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
VOTE +1


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16160953#comment-16160953
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
I've filed the tickets 1774 and 1775 for the pool enhancements.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16158731#comment-16158731
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
> Should we file a different ticket for the pool improvements?

Yes, I think a dedicated ticket for those improvements is a good idea  and 
it makes probably sense if you create it as you already made some good 
suggestions.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16158424#comment-16158424
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
About request pipelining, you can send in a queue one by one regardless of 
whether or not it was received. On the read side, once a message is received 
you issue a following call to `ReceiveAsync()` (regardless whether a message 
has been sent). That way the client can send x messages without waiting for a 
response.

Should we file a different ticket for the pool improvements?

About the changes in this pr: VOTE +1




> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16155878#comment-16155878
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
I just pushed a commit that addresses your comments, @jorgebay.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16155170#comment-16155170
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
Thanks for your comments so far, I'll implement the changes you suggested.

I definitely agree that the driver and especially the connection pool needs 
to be improved and I would really appreciate any effort you (and of course als 
anyone else) put into this as I'm not exactly an expert in writing database 
drivers.

Could you clarify what you mean with _request pipelining on the same 
connection_? If you mean sending multiple requests in parallel over the same 
websocket connection, then I think that it's not possible as [the 
documentation](https://docs.microsoft.com/en-us/dotnet/api/system.net.websockets.clientwebsocket.sendasync?view=netframework-4.7#Remarks)
 states that

> Exactly one send and one receive is supported on each ClientWebSocket 
object in parallel.

The upside of this is of course that the implementation of the `Connection` 
and the `ConnectionPool` is much simpler when every `Connection` can just send 
one single request at a time as we don't have to match incoming responses with 
the correct requests they belong to.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16155072#comment-16155072
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/704
  
I've left some line comments on the pr.

Apart from the issue this patch is addressing, we should create separate 
tickets for issues related to the .NET driver pool:
- Blocking (use of `lock`).
- An unbounded number of connections can be created under pressure.
- Lack of request pipelining on the same connection.

I wasn't able to focus on the .NET driver itself, my initial priority was 
getting a stable `GraphTraversal` API and bytecode serialization, but we can 
tackle these issues progressively in the next versions. 


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16155059#comment-16155059
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/704#discussion_r137214097
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs ---
@@ -74,6 +85,36 @@ private void AddConnection(Connection connection)
 }
 }
 
+private void ConsiderUnavailable()
+{
+CloseAndRemoveAllConnections();
+}
+
+private void CloseAndRemoveAllConnections()
+{
+lock (_connectionsLock)
+{
+TeardownAsync().WaitUnwrap();
+RemoveAllConnections();
+}
+}
+
+private void RemoveAllConnections()
+{
+while (!_connections.IsEmpty)
+{
+_connections.TryTake(out var connection);
+connection.Dispose();
+}
+}
+
+private async Task TeardownAsync()
+{
+var closeTasks = new List(_connections.Count);
--- End diff --

The list isn't necessary, you can use the `Task.WhenAll(IEnumerable)` 
overload instead: `Task.WhenAll(_connections.Select(c => c.CloseAsync()))`


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16155054#comment-16155054
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/704#discussion_r137213383
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs ---
@@ -74,6 +85,36 @@ private void AddConnection(Connection connection)
 }
 }
 
+private void ConsiderUnavailable()
+{
+CloseAndRemoveAllConnections();
+}
+
+private void CloseAndRemoveAllConnections()
+{
+lock (_connectionsLock)
+{
+TeardownAsync().WaitUnwrap();
+RemoveAllConnections();
+}
+}
+
+private void RemoveAllConnections()
+{
+while (!_connections.IsEmpty)
--- End diff --

Using `TryTake()` and `IsEmpty` can create race conditions.
Use `while (bag.TryTake(out c))` instead.


> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1766) Gremlin.Net: Closed connections should not be re-used

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16151537#comment-16151537
 ] 

ASF GitHub Bot commented on TINKERPOP-1766:
---

GitHub user FlorianHockmann opened a pull request:

https://github.com/apache/tinkerpop/pull/704

TINKERPOP-1766 Gremlin.Net: Add handling for closed connections

https://issues.apache.org/jira/browse/TINKERPOP-1766

This avoids `Connections` from being added back to the `ConnectionPool` 
when they were closed which happens when the server gets unavailable. In that 
case all existing `Connections` are removed from the `ConnectionPool` and 
closed as they are most likely also not `Open` anymore. This is in line with 
the handling for closed connections in `gremlin-driver`.

Unfortunately, I don't know of a good way to test something like this. We 
probably would have to make internal classes like `Connection` public which 
isn't that nice as users shouldn't see them in my opinion.

VOTE +1

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/apache/tinkerpop TINKERPOP-1766

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/tinkerpop/pull/704.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 #704


commit 98e4e1b8201f50e1f34458a82c20f2737aa6ae53
Author: florianhockmann 
Date:   2017-09-02T15:54:39Z

Add handling for closed connections

Closed connections are not returned to the ConnectionPool as the client 
cannot send any further requests with them. Additionally, the host is 
considered to be unavailable when a connection was closed and all other 
connections are closed and removed from the ConnectionPool. This is in line 
with the handling for closed connection in gremlin-driver.

commit d9a4f79f7c568761461dc11d22fbf907578f7d14
Author: florianhockmann 
Date:   2017-09-02T16:00:52Z

Update NuGet packages of test projects

One of the dependencies (Castle.Core) produced a warning because one of its 
dependencies couldn't be found in the specified version. This doesn't occur 
anymore for the updated version.




> Gremlin.Net: Closed connections should not be re-used 
> --
>
> Key: TINKERPOP-1766
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1766
> Project: TinkerPop
>  Issue Type: Bug
>  Components: language-variant
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Florian Hockmann
>
> The driver of Gremlin.Net is kept very simle which holds especially true for 
> the {{ConnectionPool}}. It simply returns every connection to its pool of 
> usable connections that was {{disposed}} by the client. Unfortunately, this 
> also applies in case the submit failed due to an already closed connection 
> which means that the client will get the closed connection back from the 
> {{ConnectionPool}} later and continues trying to submit messages over this 
> closed connection.
> This can be fixed by checking whether the {{Connection}} is still open before 
> adding it back to the {{ConnectionPool}}.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)