synchronizing reads to a java collection

so i want to have an arraylist that stores a series of stock quotes. but i keep track of bid price, ask price and last price for each.

of course at any time, the bid ask or last of a given stock can change.

i have one thread that updates the prices and one that reads them.

i want to make sure that when reading no other thread is updating a price. so i looked at synchronized collection. but that seems to only prevent reading while another thread is adding or deleting an entry to the arraylist.

so now i'm onto the wrapper approach:

public class Qte_List {
private final ArrayList<Qte> the_list;

public 开发者_运维技巧void UpdateBid(String p_sym, double p_bid){
    synchronized (the_list){
        Qte q = Qte.FindBySym(the_list, p_sym);
        q.bid=p_bid;}
}

public double ReadBid(String p_sym){
    synchronized (the_list){
        Qte q = Qte.FindBySym(the_list, p_sym);
        return q.bid;}
}

so what i want to accomplish with this is only one thread can be doing anything - reading or updating an the_list's contents - at one time. am i approach this right?

thanks.

Yes, you are on the right track and that should work.

But why not use the existing Hashtable collection, which is synchronized, and provides a key-value lookup already?

As I understand it you are using the map to store the quotes; the number of quotes never changes, but each quote can be read or modified to reflect current prices. It is important to know that locking the collection only protects against changes to which Quote objects are in the map: it does not in any way restrict the modification of the contents of those Quotes. If you want to restrict that access you will have to provide locking on the Quote object.

Looking at your code however I don't believe you have a significant synchronization problem. If you try to do a read at the same time as a write, you will either get the price before or the price after the write. If you didn't know the write was going to occur that shouldn't matter to you. You may need locking at a higher level so that

if (getBidPrice(mystock)<10.0) {
  sell(10000);
}

happens as an atomic operation and you don't end up selling at 5.0 rather than 10.0.

If the number of quotes really doesn't change then I would recommend allowing Qte objects to be added only in the constructor of Qte_List. This would make locking the collection irrelevant. The technical term for this is making Qte_List immutable.

That looks like a reasonable approach. Nit-picking, though, you probably shouldn't include the return statement inside the synchronized block:

public double ReadBid(String p_sym){
    double bid;
    synchronized (the_list) {
        Qte q = Qte.FindBySym(the_list, p_sym);
        bid = q.bid;
    }

    return bid;
}

I'm not sure if it's just my taste or there's some concurrency gotcha involved, but at the very least it looks cleaner to me ;-).

Yes this will work, anyway you don't need to do it yourself since it is already implemented in the Collections framework

Collections.synchronizedList

Your approach should do the trick, but as you stated, there can only be one reader and writer at a time. This isn't very scaleable.

There are some ways to improve performance without loosing thread-safety here.
You could use a ReadWriteLock for example. This will allow multiple readers at a time, but when someone gets the write-lock, all others must wait for him to finish.

Another way would be to use a proper collection. It seems you could exchange your list with a thread-safe implementation of Map. Have a look at the ConcurrentMap documentation for possible candidates.

Edit:
Assuming that you need ordering for your Map, have a look at the ConcurrentNavigableMap interface.

What you have will work, but locking the entire list every time you want to read or update the value of an element is not scalable. If this doesn't matter, then you're fine with what you have. If you want to make it more scalable consider the following...

You didn't say whether you need to be able to make structural changes to the_list (adding or removing elements), but if you don't, one big improvement would be to move the call to FindBySym() outside of the synchronized block. Then instead of synchronizing on the_list, you can just synchronize on q (the Qte object). That way you can update different Qte objects concurrently. Also, if you can make the Qte objects immutable as well you actually don't need any synchronization at all. (to update, just use the_list[i] = new Qte(...) ).

If you do need to be able to make structural changes to the list, you can use a ReentrantReadWriteLock to allow for concurrent reads and exclusive writes.

I'm also curious why you want to use an ArrayList rather than a synchronized HashMap.