Aries-2121 Add TCP provider connection persistence with connection pool#58
Open
amichair wants to merge 5 commits intoapache:masterfrom
Open
Aries-2121 Add TCP provider connection persistence with connection pool#58amichair wants to merge 5 commits intoapache:masterfrom
amichair wants to merge 5 commits intoapache:masterfrom
Conversation
added 5 commits
July 2, 2025 10:15
…Service so that resources can be properly released
…ol (~100x throughput)
alien11689
reviewed
Apr 7, 2026
| BasicObjectOutputStream out; | ||
| BasicObjectInputStream in; | ||
|
|
||
| public Connection(Socket socket) throws IOException { |
Contributor
There was a problem hiding this comment.
nitpicking: public is not necessary
| private int timeoutMillis; | ||
|
|
||
| private final Deque<Connection> pool = new ArrayDeque<>(); | ||
| private int acquired; // counts connections currently in use (not in pool) |
Contributor
There was a problem hiding this comment.
we can make the comment a javadoc
| private Connection acquireConnection() throws IOException { | ||
| Connection conn; | ||
| synchronized (pool) { | ||
| acquired++; // must be first |
| Connection conn; | ||
| synchronized (pool) { | ||
| acquired++; // must be first | ||
| if (closed) { |
Contributor
There was a problem hiding this comment.
this check may be first?
| return conn; | ||
| } | ||
|
|
||
| // must be called exactly once for each call to acquireConnection, |
Contributor
There was a problem hiding this comment.
the comments above the methods may be the javadoc
| // regardless of the outcome - if there was an error, pass null | ||
| private void releaseConnection(Connection conn) { | ||
| synchronized (pool) { | ||
| acquired--; // must be first |
Contributor
There was a problem hiding this comment.
should we have negative check?
|
|
||
| private void closeConnection(Connection conn) throws IOException { | ||
| if (conn != null) { | ||
| conn.socket.close(); |
Contributor
There was a problem hiding this comment.
- can we have method close on Connection class?
- we don't need to close in/out?
| closed = true; // first prevent acquiring new connections | ||
| while (true) { | ||
| // close all idle connections | ||
| for (Iterator<Connection> it = pool.iterator(); it.hasNext(); ) { |
Contributor
There was a problem hiding this comment.
- can we use for each normally?
for (Connection connection : pool)? - should we run in try catch for each connection?
| handleCall(invoker, in, out); | ||
| } | ||
| } catch (SocketException | SocketTimeoutException | EOFException se) { | ||
| return; // e.g. connection closed by client or read timeout due to inactivity |
| * over a TCP connection, to be executed by the remote service. | ||
| */ | ||
| public class TcpInvocationHandler implements InvocationHandler { | ||
| public class TcpInvocationHandler implements InvocationHandler, Closeable { |
Contributor
There was a problem hiding this comment.
can we test the new changes? there are not a lot of tests at the end of this PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements ARIES-2121 wtih a simple connection pool. Performance tests (when increasing the number of calls in TcpProviderTest) show an increase in throuput on the order of 100x.
Note that this PR is on top of ARIES-2120, so that PR has to be merged first.