activemq-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Bain <tb...@alumni.duke.edu>
Subject Re: Potential threading bug in MemoryMessageStore.
Date Sun, 21 Jun 2015 16:02:52 GMT
But that's the thing; I think that method is properly synchronized, and
many of the other methods are over-synchronized.

Here's the code in question: from
http://grepcode.com/file/repo1.maven.org/maven2/org.apache.activemq/activemq-osgi/5.11.1/org/apache/activemq/store/memory/MemoryMessageStore.java#MemoryMessageStore.addMessage%28org.apache.activemq.broker.ConnectionContext%2Corg.apache.activemq.command.Message%29

messageTable is a synchronized map obtained from
Collections.synchronizedMap(), so get() acquires the intrinsic lock before
issuing the get() against the underlying Map.  The various other methods in
the class explicitly acquire that intrinsic lock via synchronized
(messageTable) {} blocks, so they're each acquiring the same lock (and
hence thread-safe).  The other methods generally invoke only one other
method inside the synchronized block, and that method will acquire the same
intrinsic lock the sychronized block already holds; this works (because
intrinsic locks are reentrant), but it's unnecessary and your point about
double synchronization is accurate (but applicable to more than just
addMessage()).

I am, however, concerned that lastBatchId should probably be volatile since
it looks like it could be set from multiple threads.  Either that, or
resetBatching() and setBatch() both need to use a synchronized block.

Tim

On Sat, Jun 20, 2015 at 8:43 PM, Kevin Burton <burton@spinn3r.com> wrote:

> I didn’t.. it’s a race so if it’s happening is probably happening not that
> often .  Maybe a test could be created to try to introduce the race.
>
> On Thu, Jun 18, 2015 at 9:57 PM, Tim Bain <tbain@alumni.duke.edu> wrote:
>
> > Did you look at this any further?  Looking at the code, it looks like the
> > call will be protected without explicit synchronization by the intrinsic
> > lock on the synchronizedMap (and I think that some other methods such as
> > delete() and addMessage() that just call a method on the synchronizedMap
> > could have their synchronized blocks removed), though I might be looking
> at
> > that wrong.
> >
> > Tim
> >
> > On Mon, Apr 6, 2015 at 1:58 PM, Kevin Burton <burton@spinn3r.com> wrote:
> >
> > > Pretty sure getMessage() in MemoryMessageStore has a bug.
> > >
> > > All access to messageTable is synchronized.  this method is not.  This
> > > means that there’s a race where a message can go into the queue but the
> > > thread reading it may have a cache copy of the data structure meaning
> it
> > > would get a cache miss
> > >
> > > Also, it looks like “addMessage” is doubly synchronized.
> > >
> > >     public Message getMessage(MessageId identity) throws IOException {
> > >         return messageTable.get(identity);
> > >     }
> > >
> > > … I’m going to migrate to using a PriorityBlockingQueue for this and
> > remove
> > > all the synchronization and will try to submit a patch. Also I think
> > > PriorityBlockingQueue will lower memory usage by 40%
> > >
> > >
> > > --
> > >
> > > Founder/CEO Spinn3r.com
> > > Location: *San Francisco, CA*
> > > blog: http://burtonator.wordpress.com
> > > … or check out my Google+ profile
> > > <https://plus.google.com/102718274791889610666/posts>
> > > <http://spinn3r.com>
> > >
> >
>
>
>
> --
>
> Founder/CEO Spinn3r.com
> Location: *San Francisco, CA*
> blog: http://burtonator.wordpress.com
> … or check out my Google+ profile
> <https://plus.google.com/102718274791889610666/posts>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message