CODEWITHSUNDEEP
javamultithreadingclient
user1845768
user1845768

Reputation: 23

Java Server Client, shared variable between threads

I am working on a project to create a simple auction server that multiple clients connect to. The server class implements Runnable and so creates a new thread for each client that connects. I am trying to have the current highest bid stored in a variable that can be seen by each client. I found answers saying to use AtomicInteger, but when I used it with methods such as atomicVariable.intValue() I got null pointer exception errors.

What ways can I manipulate the AtomicInteger without getting this error or is there an other way to have a shared variable that is relatively simple?

Any help would be appreciated, thanks.

Update

I have the AtomicInteger working. The problem is now that only the most recent client to connect to the server seems to be able to interact with it. The other client just sort of freeze.

Would I be correct in saying this is a problem with locking?

Upvotes: 2

Views: 1402

Answers (3)

Bohemian
Bohemian

Reputation: 425238

I wouldn't look at the problem like that. I would simply store all the bids in a ConcurrentSkipListSet, which is a thread-safe SortedSet. With the correct implementation of compareTo(), which determines the ordering, the first element of the Set will automatically be the highest bid.

Here's some sample code:

public class Bid implements Comparable<Bid> {
    String user;
    int amountInCents;
    Date created;

    @Override
    public int compareTo(Bid o) {
        if (amountInCents == o.amountInCents) {
            return created.compareTo(created); // earlier bids sort first
        }
        return o.amountInCents - amountInCents; // larger bids sort first
    }
}

public class Auction {
    private SortedSet<Bid> bids = new ConcurrentSkipListSet<Bid>();

    public Bid getHighestBid() {
        return bids.isEmpty() ? null : bids.first();
    }

    public void addBid(Bid bid) {
        bids.add(bid);
    }
}

Doing this has the following advantages:

  • Automatically provides a bidding history
  • Allows a simple way to save any other bid info you need

You could also consider this method:

/**
 * @param bid
 * @return true if the bid was successful
 */
public boolean makeBid(Bid bid) {
    if (bids.isEmpty()) {
        bids.add(bid);
        return true;
    }
    if (bid.compareTo(bids.first()) <= 0) {
        return false;
    }
    bids.add(bid);
    return true;
}

Upvotes: 2

Neil Coffey
Neil Coffey

Reputation: 21815

Using an AtomicInteger is fine, provided you initialise it as Tomasz has suggested.

What you might like to think about, however, is whether all you will literally ever need to store is just the highest bid as an integer. Will you never need to store associated information, such as the bidding time, user ID of the bidder etc? Because if at a later stage you do, you'll have to start undoing your AtomicInteger code and replacing it.

I would be tempted from the outset to set things up to store arbitrary information associated with the bid. For example, you can define a "Bid" class with the relevant field(s). Then on each bid, use an AtomicReference to store an instance of "Bid" with the relevant information. To be thread-safe, make all the fields on your Bid class final.

You could also consider using an explicit Lock (e.g. see the ReentrantLock class) to control access to the highest bid. As Tomasz mentions, even with an AtomicInteger (or AtomicReference: the logic is essentially the same) you need to be a little careful about how you access it. The atomic classes are really designed for cases where they are very frequently accessed (as in thousands of times per second, not every few minutes as on a typical auction site). They won't really give you any performance benefit here, and an explicit Lock object might be more intuitive to program with.

Upvotes: 1

Tomasz Nurkiewicz
Tomasz Nurkiewicz

Reputation: 340903

Well, most likely you forgot to initialize it:

private final AtomicInteger highestBid = new AtomicInteger();

However working with highestBid requires a great deal of knowledge to get it right without any locking. For example if you want to update it with new highest bid:

public boolean saveIfHighest(int bid) {
    int currentBid = highestBid.get();
    while (currentBid < bid) {
        if (highestBid.compareAndSet(currentBid, bid)) {
            return true;
        }
        currentBid = highestBid.get();
    }
    return false;
}

or in a more compact way:

for(int currentBid = highestBid.get(); currentBid < bid; currentBid = highestBid.get()) {
    if (highestBid.compareAndSet(currentBid, bid)) {
        return true;
    }
}
return false;

You might wonder, why is it so hard? Image two threads (requests) biding at the same time. Current highest bid is 10. One is biding 11, another 12. Both threads compare current highestBid and realize they are bigger. Now the second thread happens to be first and update it to 12. Unfortunately the first request now steps in and revert it to 11 (because it already checked the condition).

This is a typical race condition that you can avoid either by explicit synchronization or by using atomic variables with implicit compare-and-set low-level support.

Seeing the complexity introduced by much more performant lock-free atomic integer you might want to restore to classic synchronization:

public synchronized boolean saveIfHighest(int bid) {
    if (highestBid < bid) {
        highestBid = bid;
        return true;
    } else {
        return false;
    }
}

Upvotes: 4

Related Questions