• Post Reply Bookmark Topic Watch Topic
  • New Topic

Multi threading issues with this code?  RSS feed

 
Jes Sie
Ranch Hand
Posts: 188
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi folks,

A lead developer in my team mentioned that the occasional exception is caused by multi-threading issues with this piece of code. We agree with that, but I was wondering which part of the code is the problem. Pls show me.

Also, can we solve this problem by removing static and putting sync in the init method?

 
David Weitzman
Ranch Hand
Posts: 1365
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would recommend replacing code that looks like this:



with code that looks like this:



And modify the init() method to check if it's been called before and do nothing if it has. There are two major benefits. First, less code duplication. Second, you can then synchronize the init() method and you shouldn't have anything else to worry about.

If all of the public methods call init(), there's an even better option: Put the initialization code into a static initializer block. The virtual machine will load the class into memory and run the initialization code in a thread-safe way the first time a method in the class is called.
 
Jes Sie
Ranch Hand
Posts: 188
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks David,

Your suggestions are wise.

The occasional exceptions that we've got is a NumberFormat exception. I was wondering whether is this due to java.text.NumberFormat? http://java.sun.com/j2se/1.4.2/docs/api/java/text/NumberFormat.html
Number formats are generally not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.


As it is, I think the init method (as your rightly pointed out) is bad....and as a result NumberFormat is stuffed. What do you think?
 
David Harkness
Ranch Hand
Posts: 1646
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Jes Sie:
The occasional exceptions that we've got is a NumberFormat exception.


Exactly. The number and date formatters are not thread-safe: they maintain internal state while parsing and formatting. This means that if two threads are using the same formatter simultaneously, they will interfere with each other, occassionally leading to an NFE.

The solutions are those specified: synchronize access to the formatter if contention is small, create them on the fly (somewhat expensive, but maybe not nearly so with modern VMs), create one per thread (use ThreadLocal -- this might be more expensive than creating on the fly), or create a pool (probably overkill).
 
David Weitzman
Ranch Hand
Posts: 1365
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ah, good point. I had assumed NumberFormat would probably be safe, but I see that's not the case. I would probably start by extracting calls to NumberFormat.parse() (and any other NumberFormat methods you use) into static synchronized methods in your class.

For example,



becomes



If you're using an IDE with automated refactoring it might actually be easier to create a subclass of NumberFormat called SynchronizedNumberFormat and then run a "Replace Inheritence with Delegation" refactoring. You IDE will be able to create method stubs for all the NumberFormatMethods that just call the corrisponding methods on the delegate object. If you just mark all the SynchronizedNumberFormat methods as synchronized then you can just take the exact code you have an replace



with



wherever it occurs. If the numberFormat gets used in a lot of places I might want to go with this option just to prevent missing any uses or forgetting to synchronize when you add new code that uses the numberFormat.

Another theoretical option is to give each thread its own NumberFormat instance using ThreadLocal objects to avoid contention, but I doubt that has any benefits here since all the synchronized blocks will return pretty quickly. A simpler version of the same thing would be to create (or clone) a new NumberFormat every time you need one, which can give the garbage collector a workout under high loads but usually isn't a problem. Anyway, I recommend just synchronizing access to the methods you use for now.
 
David Harkness
Ranch Hand
Posts: 1646
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Looking at the code for SimpleDateFormat gives me another idea for the curious, though I haven't run any benchmarks. The constructor and initialization methods for SDF do quite a bit of work. Some of the results are cached, but quite a few temporary small objects are created. The implementation of clone(), however, is quite simple.

So the idea is to create and initialize a master formatter and then clone it before each use. My theory is that cloning it will be far quicker than initializing a new one programmatically. If you try this, please let us know.
 
Warren Dew
blacksmith
Ranch Hand
Posts: 1332
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Clone() isn't thread safe by default either, is it? Seems to me you'd have the same synchronization issues....
 
David Harkness
Ranch Hand
Posts: 1646
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Warren Dew:
Clone() isn't thread safe by default either, is it? Seems to me you'd have the same synchronization issues....

No, clone() by itself is not thread-safe. If another thread is modifying the object being cloned, problems will arise. In this case, however, the "prototype" object created at initialization time is never modified by any thread -- it is created once and cloned over and over.

As long as clone() does not modify the object being cloned, this is safe. If it does, it is violating the contract for clone().

Here's a trimmed-down example:

To avoid massive proliferation of temporary formatters, you could make the helper class non-static and have it grab a formatter clone during construction and reuse it for subsequent operations. As long as you don't share helper objects among threads, you'll achieve single-threaded access to the formatters themselves.

Here's the same example modified as such:

[ August 24, 2004: Message edited by: David Harkness ]
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!