[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: Tinkerpop 2135 Fix handling of closed idle connections in Gremlin.Net driver

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Thanks for your review, @jorgebay!

I'm not sure if you're following on [TINKERPOP-2148](https://issues.apache.org/jira/browse/TINKERPOP-2148), but user Zaoshi suggested there to switch from `ConcurrentBag` to `ConcurrentQueue`. This would have two nice advantages for us right now:

1. We don't have to iterate over all connections and find the least used one. Instead, we can simply dequeue/enqueue connections and use the first that hasn't reached it's limit. This is effectively a round-robin mechanism which is probably more efficient than our current approach.
2. Removal of a closed connection would be as simple as not enqueueing it again during this process.

Would be good to hear your opinion on that.

What's the advantage of such a custom copy-on-write list? Only avoiding the expensive `Count` implementation of `ConcurrentDictionary`? That seems to be at least less expensive for [`ConcurrentQueue`](https://github.com/Microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/mscorlib/system/collections/Concurrent/ConcurrentQueue.cs#L394). But we would probably have to maintain our own connection counter if we moved to such a dequeue/enqueue approach as that would change the count all the time.

In general, I would prefer if we could stick with the default collection types if possible and only add our own types if we really have to.

> To ensure that concurrent modifications to the pool don't occur, e.g., pool growing and closing, we could use something like a ordered task scheduler.

I also thought about how we could prevent that, but I wanted to postpone it to a subsequent PR. A task scheduler sounds like a good solution in general. Do you know whether the default [TaskScheduler](https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskscheduler) would work here? I haven't worked with that yet myself.
With a task scheduler we would probably only have to change the way we replace dead connections. Currently, all connections are removed from the pool if a request fails due to a network error and new connections are created the next time a connection is needed. That would not work anymore if a scheduled task is used to create new connections as we probably can't really wait for that (?)

[ Full content available at: https://github.com/apache/tinkerpop/pull/1077 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org