You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@camel.apache.org by Roman Kalukiewicz <ro...@gmail.com> on 2008/04/22 13:12:37 UTC
Re: svn commit: r650379 - /activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
2008/4/22, hadrian@apache.org <ha...@apache.org>:
> Author: hadrian
> Date: Mon Apr 21 21:26:05 2008
> New Revision: 650379
>
> URL: http://svn.apache.org/viewvc?rev=650379&view=rev
> Log:
> Fix thread safety issue.
>
> Modified:
> activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
>
> Modified: activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
> URL: http://svn.apache.org/viewvc/activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java?rev=650379&r1=650378&r2=650379&view=diff
> ==============================================================================
> --- activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java (original)
> +++ activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java Mon Apr 21 21:26:05 2008
> @@ -58,7 +58,7 @@
> private static final String DEFAULT_QUEUE_BROWSE_STRATEGY = "org.apache.camel.component.jms.DefaultQueueBrowseStrategy";
> private JmsConfiguration configuration;
> private ApplicationContext applicationContext;
> - private Requestor requestor;
> + private volatile Requestor requestor;
> private QueueBrowseStrategy queueBrowseStrategy;
> private boolean attemptedToCreateQueueBrowserStrategy;
>
> @@ -314,8 +314,10 @@
>
> public synchronized Requestor getRequestor() throws Exception {
> if (requestor == null) {
> - requestor = new Requestor(getConfiguration(), getExecutorService());
> - requestor.start();
> + synchronized (this) {
> + requestor = new Requestor(getConfiguration(), getExecutorService());
> + requestor.start();
> + }
> }
> return requestor;
> }
The method is synchronized anyway so we don't need
'synchronized(this)' I believe.
If it was not synchronized we anyway could start requestor twice (as
two threads check requestor == null, then both could create and start
new requestor.
I believe the code was fine before and we don't need this change.
Roman
Re: svn commit: r650379 - /activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
Posted by James Strachan <ja...@gmail.com>.
Agreed; the entire method has to be synchronized to avoid double
checked logging issues
On 22/04/2008, Roman Kalukiewicz <ro...@gmail.com> wrote:
> 2008/4/22, hadrian@apache.org <ha...@apache.org>:
>
> > Author: hadrian
> > Date: Mon Apr 21 21:26:05 2008
> > New Revision: 650379
> >
> > URL: http://svn.apache.org/viewvc?rev=650379&view=rev
> > Log:
> > Fix thread safety issue.
> >
> > Modified:
> > activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
> >
> > Modified: activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java
> > URL: http://svn.apache.org/viewvc/activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java?rev=650379&r1=650378&r2=650379&view=diff
> > ==============================================================================
> > --- activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java (original)
> > +++ activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java Mon Apr 21 21:26:05 2008
> > @@ -58,7 +58,7 @@
> > private static final String DEFAULT_QUEUE_BROWSE_STRATEGY = "org.apache.camel.component.jms.DefaultQueueBrowseStrategy";
> > private JmsConfiguration configuration;
> > private ApplicationContext applicationContext;
> > - private Requestor requestor;
> > + private volatile Requestor requestor;
> > private QueueBrowseStrategy queueBrowseStrategy;
> > private boolean attemptedToCreateQueueBrowserStrategy;
> >
> > @@ -314,8 +314,10 @@
> >
> > public synchronized Requestor getRequestor() throws Exception {
> > if (requestor == null) {
> > - requestor = new Requestor(getConfiguration(), getExecutorService());
> > - requestor.start();
> > + synchronized (this) {
> > + requestor = new Requestor(getConfiguration(), getExecutorService());
> > + requestor.start();
> > + }
> > }
> > return requestor;
> > }
>
>
> The method is synchronized anyway so we don't need
> 'synchronized(this)' I believe.
> If it was not synchronized we anyway could start requestor twice (as
> two threads check requestor == null, then both could create and start
> new requestor.
>
> I believe the code was fine before and we don't need this change.
>
>
> Roman
>
--
James
-------
http://macstrac.blogspot.com/
Open Source Integration
http://open.iona.com