-
1. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 4:48 AM (in response to alesj)I guess if I just write lock the deploy and undeploy lists processing, you should be fine.
Index: deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java =================================================================== --- deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java (revision 84459) +++ deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java (working copy) @@ -322,33 +322,41 @@ @@ -623,13 +631,13 @@ if (deployers == null) throw new IllegalStateException("No deployers"); + List<DeploymentContext> undeployContexts = null; lockWrite(); try { if (shutdown.get()) throw new IllegalStateException("The main deployer is shutdown"); - List<DeploymentContext> undeployContexts = null; if (undeploy.isEmpty() == false) { // Undeploy in reverse order (subdeployments first) @@ -640,21 +648,33 @@ Collections.sort(undeployContexts, reverted); undeploy.clear(); } - if (undeployContexts != null) - { - deployers.process(null, undeployContexts); - } + } + finally + { + unlockWrite(); + } - try - { - processToDeploy(); - } - catch (DeploymentException e) - { - throw new RuntimeException("Error while processing new deployments", e); - } + if (undeployContexts != null) + { + deployers.process(null, undeployContexts); + } - List<DeploymentContext> deployContexts = null; + try + { + processToDeploy(); + } + catch (DeploymentException e) + { + throw new RuntimeException("Error while processing new deployments", e); + } + + List<DeploymentContext> deployContexts = null; + lockWrite(); + try + { + if (shutdown.get()) + throw new IllegalStateException("The main deployer is shutdown"); + if (deploy.isEmpty() == false) { deployContexts = new ArrayList<DeploymentContext>(deploy); @@ -662,15 +682,16 @@ Collections.sort(deployContexts, comparator); deploy.clear(); } - if (deployContexts != null) - { - deployers.process(deployContexts, null); - } } finally { unlockWrite(); } + + if (deployContexts != null) + { + deployers.process(deployContexts, null); + } } public DeploymentStage getDeploymentStage(String deploymentName) throws DeploymentException
-
2. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 7:48 AM (in response to alesj)"alesj" wrote:
I guess if I just write lock the deploy and undeploy lists processing, you should be fine.
This is not fully sufficient either,
as one could do addDeployment in between the two write locks in process(),
and if this new deployment would actually be re-deploy,
we would only pick up its deploy part, but not the undeploy part. :-( -
3. Re: Locks on MainDeployerImpl
adrian.brock Feb 20, 2009 7:56 AM (in response to alesj)"alesj" wrote:
"alesj" wrote:
I guess if I just write lock the deploy and undeploy lists processing, you should be fine.
This is not fully sufficient either,
as one could do addDeployment in between the two write locks in process(),
and if this new deployment would actually be re-deploy,
we would only pick up its deploy part, but not the undeploy part. :-(
No. The process() uses snapshots of the to(Un)Deploy.
If two threads concurrently do:
add(remove)Deployment()
process()
then they might end up processing each other's stuff but they won't miss one. -
4. Re: Locks on MainDeployerImpl
adrian.brock Feb 20, 2009 7:57 AM (in response to alesj)As commented on the jboss-development list, only shutdown should be using the
write lock. -
5. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 8:05 AM (in response to alesj)"adrian@jboss.org" wrote:
then they might end up processing each other's stuff but they won't miss one.
I wasn't thinking about missing one.
I had this in mind (#t = #thread):
1t - addDeployment
1t - process
While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
Meaning it will add its previous context to undeploy, which wouldn't be
processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
which can lead to whatever. :-) -
6. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 8:15 AM (in response to alesj)"alesj" wrote:
"adrian@jboss.org" wrote:
then they might end up processing each other's stuff but they won't miss one.
I wasn't thinking about missing one.
I had this in mind (#t = #thread):
1t - addDeployment
1t - process
While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
Meaning it will add its previous context to undeploy, which wouldn't be
processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
which can lead to whatever. :-)
I probably have to create similar undeploy processing
(and not do that in addDeployment)
in process() as I do for deploy in processToDeploy. -
7. Re: Locks on MainDeployerImpl
adrian.brock Feb 20, 2009 8:27 AM (in response to alesj)"alesj" wrote:
"adrian@jboss.org" wrote:
then they might end up processing each other's stuff but they won't miss one.
I wasn't thinking about missing one.
I had this in mind (#t = #thread):
1t - addDeployment
1t - process
While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
Meaning it will add its previous context to undeploy, which wouldn't be
processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
which can lead to whatever. :-)
Ok, I see your point, but that's not a reason to use the write lock which is for a different purpose.
I can see two possible fixes:
1) Have a seperate toRedeploy list you can snapshot where "processUndeploy()"
adds those to the "toUndeploy" snapshot and creates an entry in the real "toDeploy" list.
2) Don't move a "toDeploy' request into the snapshot while it exists in the
real "toUndeploy" list. -
8. Re: Locks on MainDeployerImpl
adrian.brock Feb 20, 2009 8:43 AM (in response to alesj)"adrian@jboss.org" wrote:
I can see two possible fixes:
1) Have a seperate toRedeploy list you can snapshot where "processUndeploy()"
adds those to the "toUndeploy" snapshot and creates an entry in the real "toDeploy" list.
2) Don't move a "toDeploy' request into the snapshot while it exists in the
real "toUndeploy" list.
However, I guess those also contain potential race conditions.
The main issue being that the snapshoting of toDeploy and toUndeploy are not
in an atomic block together.
So a more complete fix would be to have just one list of "deployment actions"
that you can snapshot and then sort those actions such that the undeploys come first
in the snapshot list. -
9. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 9:03 AM (in response to alesj)What about if I just simply do this:
public void process() { if (deployers == null) throw new IllegalStateException("No deployers"); lockRead(); try { if (shutdown.get()) throw new IllegalStateException("The main deployer is shutdown"); Map<String, Deployment> copy = new LinkedHashMap<String, Deployment>(toRedeploy); toRedeploy.clear(); try { processToUndeploy(copy.keySet()); } catch (DeploymentException e) { throw new RuntimeException("Error while removing re-deployments", e); } List<DeploymentContext> undeployContexts = null; if (undeploy.isEmpty() == false) { // Undeploy in reverse order (subdeployments first) undeployContexts = new ArrayList<DeploymentContext>(undeploy.size()); for (int i = undeploy.size() - 1; i >= 0; --i) undeployContexts.add(undeploy.get(i)); if (reverted != null) Collections.sort(undeployContexts, reverted); undeploy.clear(); } if (undeployContexts != null) { deployers.process(null, undeployContexts); } try { processToDeploy(copy.values()); } catch (DeploymentException e) { throw new RuntimeException("Error while adding -redeployments", e); } List<DeploymentContext> deployContexts = null; if (deploy.isEmpty() == false) { deployContexts = new ArrayList<DeploymentContext>(deploy); if (comparator != null) Collections.sort(deployContexts, comparator); deploy.clear(); } if (deployContexts != null) { deployers.process(deployContexts, null); } } finally { unlockRead(); } }
Where I make a copy of currently existing re-deployments.
Then I don't care if some come in between.
Or am I missing something? -
10. Re: Locks on MainDeployerImpl
brian.stansberry Feb 20, 2009 11:14 AM (in response to alesj)A nit; you can lose changes between the copy and the clear. Better is:
Map<String, Deployment> copy = null; synchronized(toRedeploy) { copy = new LinkedHashMap<String, Deployment>(toRedeploy); toRedeploy.clear(); }
-
11. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 1:51 PM (in response to alesj)Shouldn't then any usage of toRedeploy be in synchronized(toRedeploy) block?
-
12. Re: Locks on MainDeployerImpl
brian.stansberry Feb 20, 2009 2:53 PM (in response to alesj)No, you don't need to because you are using Collections.synchronizedMap. The map it returns synchronizes on itself in all methods to prevent concurrent access. So synchronizing on it will prevent other threads mutating it in the middle of the two calls.
There's some obscure aspect to this I won't go into details here (unless you want), but for reference, this is discussed in detail in section 5.1.1 of Java Concurrency in Practice.
You could make toRedeploy a plain HashMap and then put all the calls on it in a synchronized block. That would have the same effect and would be more understandable. -
13. Re: Locks on MainDeployerImpl
brian.stansberry Feb 20, 2009 2:58 PM (in response to alesj)For the record; I wasn't clear above; you don't need to synchronize in the other places toRedeploy is used because only a single call is made. If in any place you made 2 calls that were meant to be a single unit of work you would need to synchronize on the map.
-
14. Re: Locks on MainDeployerImpl
alesj Feb 20, 2009 3:00 PM (in response to alesj)That's what I meant. :-)