• Post Reply Bookmark Topic Watch Topic
  • New Topic

In ConnectionPoolManager how synch two diff Lists?  RSS feed

 
Dan Bizman
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have written a connection pool manager and I have set a max number of (physical) connections possible. I also have idle connections in one List (an ArrayList) and active connections in another List. So my questions pertain to synchronizing on these two lists:
1. When I'm asked to add a new connection, I have to make sure it is within the max number allowed. So I have to check the size of the idle List and the active List. In order to synch on both these (to ensure neither changes before I've added the connections and that the size of both are collectively within the max amount), I would need nested synchronized blocks like this:

However, this seems to me to be a possibility of deadlock since in other methods you can get a lock on one or the other Lists. Is this the case? (Info: While there are other cases in my code where you can get a lock on idleConnections and activeConnections, it is ALWAYS either just a lock on idle, just a lock on active, or a lock in the same order as here: idleConnections and then inside that activeConnections)
2. Is there a way to have these two lists, be able to synch on them both at the same time (avoiding deadlock) to add the connections and ensure that throughout that whole adding process (create physical conn, add it to list) that no one else adds one or removes from the list?
3. Is there a better way to do this? I was thinking of making a "wrapper" class for the connections called PooledConnectionWrapper that would hold the connection and have method isActive(). But then to find an idle or active connection, I'd have to loop through every time and that's not good performance. Which is why I put them in diff. Lists. So what's the solution?
[ March 02, 2003: Message edited by: Dan Bizman ]
 
Michael Morris
Ranch Hand
Posts: 3451
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Dan,
Well you could synchronize on a dedicated lock Object or create a LockManager which would probably be safer. A simple (crude?) LockManager would look something like this:

So the trick is to call lockManager.lock() before working with your Lists and call lockManager.unlock() when you finish. You must be sure to balance your lock and unlock calls to prevent deadlock.
This topic should be in Threads and Synchronization. I'm going to move it there.
Michael Morris
 
Dan Bizman
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sure, but doesn't that mean that if I need to put something into just one of the Lists, that both lists are locked? Is that not too big of a deal?
 
Michael Morris
Ranch Hand
Posts: 3451
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Dan,

Sure, but doesn't that mean that if I need to put something into just one of the Lists, that both lists are locked?

Yep.

Is that not too big of a deal?

That depends. You could take a performance hit if you had a lot threads wanting access to the Lists. Other than that it is probably the safest approach. Of course, you could redesign the LockManager class to lock either or both Lists. Maybe something like this:

Hope this helps,
Michael Morris
 
Dan Bizman
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks for the code! It'll take a while to try that out and see how it plays out and changes my code, but I'll get back to you with my findings.
Right now I think my code is actually all working well (and I make sure that when I lock both it's ALWAYS in the same order, otherwise I lock only one and when that happens I make sure that it never calls nor is called from another method that locks the other. So that deadlock should never occur (is that correct?).
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yes, I think your original idea is correct - you're safe from deadlocks here as long as no code ever tries to acquire these locks in the opposite order. A long as the two List objects are private and the only code which ever acquires locks on them is in this class and thus under your control, you should be OK. Make sure that once you acquire locks in your code you don't make calls on any method of outside classes which might require synchronization on other objects, or you're asking for trouble.
I'm afraid I don't see the point of Michael's locking code. It doesn't seem to offer any significant benefits over standard Java synchronized locks, and has several new opportunities for error. Most importantly, the current lock() method allows multple threads to think they have acquired a lock on a given object, all at the same time. Consider - Thread A executes lock() and sets locked = true. Now threads B, C, D, E all execute lock() and find themselves in the wait() method. Then A executes unlock(), and all waiting threads are notified. One at a time, they leave the wait() method and set locked to true - even if it's already true. I.e. they don't check to see if any other threads have acquired the lock before proceeding. This bug can be fixed by replacing "if (locked)" with "while (locked)". However there are other issues - like the fact that unlike synchronized locks, the lock() method is not reentrant. Or the fact that having separate lock() and unlock() methods (rather than a single synchronized block or method) makes it much easier to acquire a lock and later fail to release it if you're not careful.
I think that, if you want to most easily avoid the deadlock complications Dan is worried about, the preferred solution is to simply have all relevant methods or blocks synchronize on the connection pool manager object, rather than synchronizing on activeConnections &/or idleConnections separately. Just like Michael's code seems intended to do, this prevents deadlocks entirely, at the cost of reduced concurrency (you can't have one thread working with idleConnections while another works with activeConnections). There's no need for a separate lock object for this - the existing connection manager object will work fine, using standard synchronization techniques.
If you do want to create your own locking class, it might as well offer some new methods not available with standard Java locks. Most useful would be methods to inquire if a lock is currently available, without blocking if it's not available. Or a lock() method with timeout, so that you can possible recover from deadlock situations. Rather than reinventing the wheel though, it might be best to check out Doug Lea's util.concurrent package. Note also that it's likely something much like this will be in JDK 1.5 thanks to JSR 166 - see this possible API.
 
Michael Morris
Ranch Hand
Posts: 3451
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Most importantly, the current lock() method allows multple threads to think they have acquired a lock on a given object, all at the same time.

Hey I said it was crude. The if (locked) block should be changed to while(locked), that was a logic error. But you are right that there really is no need for a lock manager in this case. It would only be an asset where you had many objects to protect and you wanted to centralize access to those objects. And of course the above code would only be a starting point.
Michael Morris
 
Dan Bizman
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jim,
Thanks for the thoughts! I agree with what you say, but I do see a benefit (in something like a pool manager) for not synchronizing the methods and instead doing it on the List objects: I can synch on just one if needed, and anyone who needs to reach the other List still can. This won't happen all the time, but the times it does saves some in performance speed (one less lock, no need to wait). Granted I have to watch every piece of code to be sure it doesn't grab locks in the wrong order, but that's just paying attention that I wrote proper code. And yes, i do have to be careful that it doesn't get caught out in other code that aquires a lock that I might try to get elsewhere. the only place this might occur in my code is where I create the PooledConnection from whatever datasource I'm managing. I've checked and it is only called from two places, both of which will never overrun each other (one is only called in a shutdown hook and has a check to be sure it can run anyways). So I believe I'm ok. Am I right in this assessment and that I save a teeny bit of processing/waiting time?
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I agree on all counts. Except I can imagine some situations where your approach might save a substantial amount of time, not just a tiny amount. Especially if the operation requiring syncing on both lists is rare, but the operations involving syncing on just one list are both common. So sure, if you're confident you're not going to acquire locks in the wrong order (and that no one else will be trying to modify the class later without properly understanding it), then go for it.
 
It is sorta covered in the JavaRanch Style Guide.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!