• Post Reply Bookmark Topic Watch Topic
  • New Topic

Connection Pool implemented using wait/ notify  RSS feed

 
Ranch Hand
Posts: 231
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi.

I am currently reading java thread intercommunication. I worked an example of connection pool. Please check it and tell me any mistakes i have done. According to me it is working fine.

A ConnectionManager singleton class which returns a list of 2 connections.



And then the Pool class



and then client app



Please check the whole code especially thread intercommunication. Post any questions so that i can anaylze on those things.
 
Bartender
Posts: 4181
22
IntelliJ IDE Java Python
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A couple of coding issues:

1) Your ConnectionManager 'singleton' may not be a singleton - its getInstance() method is not properly synchronized, so it could produce multiple ConnectionManagers. I don't see why you need lazy initialization for the ConnectionManager, so if I were you I would instantiate the instance in-place (so you don't have to worry about synchronization and thread safe lazy initialization).

2) Your PoolThread's getConnectionFromList() method may return a null connection if the wait() was interrupted. That is a bad thing. I would put the try/catch inside the while loop so if the wait() is interrupted it goes back to check if list isEmpty() and if it is continues to wait.

3) In the PoolThread's close() method, you catch the IllegalMonitorStateException. This exception is (a) impossible to get in your code as written and (b) indicates an un-recoverable programming error. It should not be caught. When you see that during testing it means you have to fix the programming error (there is no way to fix it at run time).

4) The standard practice for forced actions, like closing a connection, is to put the action in a finally block. So in your client code I would do:


This allows you to:
- be sure the close always happens
- catch only meaningful exceptions (you aren't forced to catch(Exception e) which could let code continue even in the event when you can't fix the problem, and continuing will mean other problems)


There are also a number of other, style or architecture problems with your code:
1) Your ConnectionManager has a getConnection() method, which actually returns a List<Connection>. This is a bad idea, either getConnection() should return a Connection or the method name should be changed to reflect that you are getting a list of connections.

2) On a related note: your ConnectionManager always generates a List of Connections, the ConnectionManager creates and defines the number of Connections that are made. This is usually the job of the connection pool (in your case the PoolThread) - the ConnectionManager should be used only to create connections, and the PoolThread should be responsible for creating and maintaining the list of connections (including the size of the list.) What I would suggest is the ConnectionManager getConnection() method would return a single connection, and the PoolThread would create a List<Connection> and call ConnectionManager.getConnection() a couple of times to fill the List.

3) Your PoolThread class is poorly named. It is not a Thread (a stream of executing code0, it is an object which holds some connections and distributes them when they are ready. Calling it something more descriptive of what it is (like ConnectionPool) would be best.

4) In PoolThread, the method getConnectionFromList() gives too much away - there is no need to make it known the connections are stored in a List. A simple getConnection() or getConnectionFromPool() would be better (it hides the implementation detail of there being a List involved).

 
Steve Luke
Bartender
Posts: 4181
22
IntelliJ IDE Java Python
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oh, and in your Pool (client) class:
I know this is just a test case, but you have a single runnable (pool) you share in all your Threads. That means all Threads share the same Connection reference. This could lead to unexpected behavior, like one Thread getting a new connection and another thread closing it before the first one has a chance to use it. There is no reason to let that connection leak out to the instance, you should define it in the run() method and keep it at the local scope to avoid this situation.



And you should provide some way for the PoolThread to 'shut down' and close one or all connections in the pool. Not doing so could lead to resource leaks on the database (open connections that are never closed) or Connections left in a bad state.


These things keep coming to me, sorry to keep harping it meant constructively...

I think you need to be a little more pro-active in cleaning up the connection in the PoolThread's close() method. You do not do anything to make sure all queries are closed or transactions complete. I am not a big database guy so I am not sure what the proper steps should be, but you need some code to make sure that any previous use of the connection is completed safely and not able to interfere with the next use.

I would also think about making a sub-class of Connection which wraps a Connection returned from the pool. You could use its close() method to call the PoolThread.close() method with the wrapped connection, but could also make it illegal to call other methods on the Connection after it is closed. In your situation, someone could call PoolThread.close(Connection) then keep using the same Connection. This could lead to conflict.
 
vipul bondugula
Ranch Hand
Posts: 231
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Steve Luke.....


Thank you very much. I need to increase my coding skills. Thanks for explaining bit by bit..I will enhance connection pooling and will post again....

Thanks once again...

Thanks
Vipul Kumar.
 
vipul bondugula
Ranch Hand
Posts: 231
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Steve Luke wrote:
I don't see why you need lazy initialization for the ConnectionManager, so if I were you I would instantiate the instance in-place (so you don't have to worry about synchronization and thread safe lazy initialization).




I am caching the ConnectionManager object so that there is no need to create for the other threads.
 
Steve Luke
Bartender
Posts: 4181
22
IntelliJ IDE Java Python
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
vipul bondugula wrote:
Steve Luke wrote:
I don't see why you need lazy initialization for the ConnectionManager, so if I were you I would instantiate the instance in-place (so you don't have to worry about synchronization and thread safe lazy initialization).




I am caching the ConnectionManager object so that there is no need to create for the other threads.


Yes, but you can do that like this:


Since initializing the static context is an atomic action associated with loading the class, this ensures the ConnectionManager is created exactly once and is thread safe. You only need to defer the instance creation if the instance creation is expensive or dependent on something that you do not have access to when the class is being loaded. And if you need that then you need to do more work to make sure the getInstance() is thread safe.

As written in your code, if two threads call getInstance() at near the same time they could produce two different ConnectionManagers, which undoes the entire purpose of the Singleton. You would need to synchronize the getInstance() method to make it work (at the least). But since there is no reason to defer instance creation (that is no reason to wait to create the instance until the getInstance() method is called - which is also called lazy initialization) you don't have to worry about that, just inline the instance creation with the static variable declaration.
 
Steve Luke
Bartender
Posts: 4181
22
IntelliJ IDE Java Python
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just as a footnote, I don't see why you need to use a Singleton for the ConnectionManager at all, it doesn't hold any data so you could just make getConnection() static and save yourself the getInstance() call (and one Object, for what that's worth). Or even assuming it did (for example a different implementation might make the connection string a variable, or something you could pass to it during instance creation), none of the data it would hold is resource limiting so why limit yourself to just one instance?

I think this is a use of a 'Pattern' just because you know the pattern and not because it fits your situation.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!