Q:

multi threading synchronization issue

I have an issues with a threading exercise in school, we are just learning about threads/sychronization and I am a bit lost.
Basically I want to start 4 threads of a certain class. Each thread will run 25 times and print out numbers 1---100.
This is what the output should look something like
thread 1: 1
Thread 3: 2
Thread 4: 3
Thread 2: 4
.
.
.
.
Thread 3: 100
But what actually gets printed out is something like this. It counts but the numbers are not correctly spitted out.
thread 1: 1
Thread 3: 1
Thread 4: 5
Thread 2: 6
Thread 3: 7
Thread 3: 8
Thread 1: 9
Thread 2: 10
Thread 4: 10
.
.
.
.
Thread 3: 100
this is my code for the class that is being threaded
public class numberentrycreater implements Runnable
{
    final private int max = 100;
    private static int count = 1;
    private String createdBy;
    private SharedBuffer buffer;

    public numberentrycreater(String createdBy ){
        this.createdBy = createdBy;
    }

    public synchronized void  increment(NumberEntry ne){
        System.out.println(ne.getCreatedBy()+": " + ne.getValue());
        if(count<=max){
            count++;
        }
    }

    @Override
    public  void run()
    {
        for(int i=0; i<25; i++){
            if(count<=max){
                NumberEntry ne = new NumberEntry(count,createdBy);
                increment(ne);
                try
                {
                    Thread.sleep(100);
                } catch (InterruptedException e)
                {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }
    }
}

I believe the issue lies in the creation of the NumberEntry Object. But I'm not quite sure how to fix it. If anyone could help me out in any way that would be great :).

A:

The synchronization is a bit of a mess here, and you're getting a lot of race conditions.
Firstly, you're creating the NumberEntry before you go into the synchronized method, so there's no reason to think that multiple threads won't pick up the same number.
Also, the synchronized method won't actually be synchronized. synchronized as a method option is just shorthand for synchronized(this) { ... }, and this in this case is the runnable, unique to each thread, so they can all enter it at the same time. You need to synchronize on something that they share. Try putting a block inside the method of synchronized(numberentrycreater.class) { <method code> } instead.
Edit:
ControlAltDel's suggestion of using a next() synchronized method rather than increment is more elegant and less likely to have problems. It still suffers from the problem of being synchronized on the wrong thing (at the time of writing) though.

