views:

264

answers:

5

Hi all,

I think I found more bugs in my web application. Normally, I do not worry about concurrency issues, but when you get a ConcurrentModificationException, you begin to rethink your design.

I am using JBoss Seam in conjuction with Hibernate and EHCache on Jetty. Right now, it is a single application server with multiple cores.

I briefly looked over my code and found a few places that haven't thrown an exception yet, but I am fairly sure they can.

The first servlet filter I have basically checks if there are messages to notify the user of an event that occurred in the background (from a job, or another user). The filter simply adds messages to the page in a modal popup. The messages are stored on the session context, so it is possible another request could pull the same messages off the session context.

Right now, it works fine, but I am not hitting a page with many concurrent requests. I am thinking that I might need to write some JMeter tests to ensure this doesn't happen.

The second servlet filter logs all incoming requests along with the session. This permits me to know where the client is coming from, what browser they're running, etc. The problem I am seeing more recently is on image gallery pages (where there are many requests at about the same time), I end up getting a concurrent modification exception because I'm adding a request to the session.

The session contains a list of requests, this list appears to be being hit by multiple threads.

@Entity
public class HttpSession 
{
  protected List<HttpRequest> httpRequests;

  @Fetch(FetchMode.SUBSELECT)
  @OneToMany(mappedBy = "httpSession")
  public List<HttpRequest> getHttpRequests()
  {return(httpRequests);}

  ...
}

@Entity
public class HttpRequest
{
  protected HttpSession httpSession;

  @ManyToOne(optional = false)
  @JoinColumn(nullable = false)
  public HttpSession getHttpSession()
  {return(httpSession);}

  ...
}

In that second servlet filter, I am doing something of the sort:

httpSession.getHttpRequests().add(httpRequest);
session.saveOrUpdate(httpSession);

The part that errors out is when I do some comparison to see what changed from request to request:

for(HttpRequest httpRequest:httpSession.getHttpRequests())

That line there blows up with a concurrent modification exception.

Things to walk away with: 1. Will JMeter tests be useful here? 2. What books do you recommend for writing web applications that scale under concurrent load? 3. I tried placing synchronized around where I think I need it, ie on the method that loops through the requests, but it still fails. What else might I need to do?

I added some comments:

I had thought about making the logging of the http requests a background task. I can easily spawn a background task to save that information. I am trying to remember why I didn't evaluate that too much. I think there is some information that I would like to have access to on the spot.

If I made it asynchronous, that would speed up the throughput quite a bit - well I'd have to use JMeter to measure those differences.

I would still have to deal with the concurrency issue there.

Thanks,

Walter

A: 

It's been caused because the list has been modified by another request while you're still iterating over it in one request. Replacing the List by ConcurrentLinkedQueue (click link to see javadoc) should fix the particular problem.

As to your other questions:

1: Will JMeter tests be useful here?

Yes, it is certainly useful to stress-test webapplications and spot concurrency bugs.

2: What books do you recommend for writing web applications that scale under concurrent load?

Not specific tied to webapplications, but more to concurrency in general, the book Concurrency in Practice is the most recommended one in that area. You can perfectly apply the learned things on webapplications as well, they are a perfect real world example of "heavy concurrent" applications.

3: I tried placing synchronized around where I think I need it, ie on the method that loops through the requests, but it still fails. What else might I need to do?

You basically need to synchronize any access to the list on the same lock. But just replacing by ConcurrentLinkedQueue is easier.

BalusC
Unfortunately, it looks like Hibernate + Javassist don't like Queues. I might be mistaken, but I think I may need to find another approach.
`Collections#synchronizedList()` should indeed do.
BalusC
A: 
  1. I'm not sure about scaling web applications in particular, but Java Concurrency in Practice is a fantastic book on concurrency in general.
  2. The list should be replaced with a version that is threadsafe, or all access to it has to be synchronized (readers and writers) on the same object. It is not enough to synchronize just the method that reads from the list.
Marcel Wolf
+4  A: 

A ConcurrentModificationException occurrs when any collection is modified while iterating over it. You can do it in a single thread, e.g.:

for( Object o : someList ) {
  someList.add( new Object() );
}

Wrap your list with Collections.synchronizedList or return an unmodifiable copy of the list.

harschware
I'm giving this a try, this looks more promising since Hibernate prefers lists or sets and both of those are supported.
Looks like this is working - I still have some work cut out for me ... My servlet filters need an overhaul. I think some of them were masking this problem.
A: 

You're getting an exception on the iterator, because another thread is altering the collection backing the iterator while you're in mid-iteration.

  • You could wrap access to the list in synchronized access (both adding and iterating) but there are problems with this, as it could take significantly longer to iterate through a list, along with the processing that goes along with it, and you'd be holding the lock to the list for all of that time.

  • Another option would be to copy the list and pass out the copy for iteration, which might be a better idea if the objects are small, as you'd only be holding the lock while you make the copy, rather than while you're iterating through the list.

  • Store your values in a ConcurrentHashMap, which uses lock striping to minimize lock contention. You could then have your get method return a copied list of the keys you want, rather than the complete objects, and access them one at at time directly from the map.

As is mentioned in another answer here, Java Concurrency in Practice is a great book.

Steve B.
A: 

The other posters are correct in stating that you need to be writing to a threadsafe data structure. In doing so, you may slow down your response time due to thread contention. Since this essentially a logging operation that is a side effect of the request itself (Or am I not understanding you correctly?) you could spawn a new thread responsible for writing to the threadsafe data structure. That allows you to proceed with the actual response instead of burning response time on a logging operation. It might be worth investigating setting up a threadpool to reduce the time required to use the logging threads.

Any concurrency book by Doug Lea is worth reading.

stinkymatt