fix for issue #715 - ensure cached consul connections are specific to…

[miklish]

… a thread to avoid concurrent use of the connections across threads.

See issue #715 for details.

[codecov-commenter]

Codecov Report

Merging #716 into 1.6.x will not change coverage.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##              1.6.x     #716   +/-   ##
=========================================
  Coverage     49.73%   49.73%           
  Complexity     1915     1915           
=========================================
  Files           261      261           
  Lines         11738    11738           
  Branches       1663     1663           
=========================================
  Hits           5838     5838           
  Misses         5209     5209           
  Partials        691      691           

Impacted Files	Coverage Δ	Complexity Δ
.../com/networknt/consul/client/ConsulClientImpl.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d331a2...ddb88f0. Read the comment docs.

[stevehu]

@miklish It looks good to me. I found a method needsToCreateConnection called from the send method to ensure that the connection is open before using it. Thanks.

[GavinChenYan]

It looks good. Thanks.

But if local thread is closed, the connection will be still in the cache map. Even the local thread ill be reused by JVM, but in special case the connection cache map could be group to big size:

private ConcurrentHashMap<String, ConsulConnection> connectionPool = new ConcurrentHashMap<>();

I would like suggest to add a feature to clear the connection pool cache if it reach config size (for example, 1000), we clear the map and let it rebuild by new connection:

connectionPool.clear()

[miklish]

@miklish It looks good to me. I found a method needsToCreateConnection called from the send method to ensure that the connection is open before using it. Thanks.

@stevehu Yes, I saw that too. That is why I didn't add any code in getConnection to check if cached connections were still alive (as we had discussed yesterday).

[GavinChenYan]

the default max work threads size is 200, maybe we are find with the cache size

[miklish]

It looks good. Thanks.

But if local thread is closed, the connection will be still in the cache map. Even the local thread ill be reused by JVM, but in special case the connection cache map could be group to big size:

private ConcurrentHashMap<String, ConsulConnection> connectionPool = new ConcurrentHashMap<>();

I would like suggest to add a feature to clear the connection pool cache if it reach config size (for example, 1000), we clear the map and let it rebuild by new connection:

connectionPool.clear()

If the HTTP connection inside the ConsulConnection object is not used for a long time, I believe either undertow or Consul will kill the connection. So only the ConsulConnection object will remain holding a closed connection.

If later (after the connection has been closed), and a thread requests that same ConsulConnection object, the ConsulConnection object first checks the status of the connection it holds, and will create a new one if needed.

[miklish]

@chenyan71 @jsu216 Can you folks approve today? @chenyan71 You have a tight timeline... it appears you are OK with the changes. If you could approve asap it would be appreciated.

[miklish]

@chenyan71 @jsu216 Nevermind, I see that this has been merged already :)