3
\$\begingroup\$

I am trying to implement a singleton builder that would be shared across multiple builder threads for Coordinate objects.

Here is the simplified target class Coordinate with only two Integer fields.

public class Coordinate
{
    private final Integer x;
    private final Integer y;

    private Coordinate(CoordinateBuilder aBuilder)
    {
        x = aBuilder.x;
        y = aBuilder.y;
    }
}

CoordinateBuilder is a static inner class inside Coordinate.class and has two blocking methods for assigning fields x and y of the current Coordinate instance being built. When a thread calls one of them, the method blocks until the other field is assigned by another thread. Both threads should return together with the Coordinate that has just been created. If the current Coordinate object is being built, other threads should wait until the current target is created and returned to callers.

/* inside Coordinate.class */
public static class CoordinateBuilder
{
    private final static CoordinateBuilder builder = new CoordinateBuilder();

    private final ReentrantLock lock = new ReentrantLock();

    private final Condition builderBusy; // cond. busy with building current instance
    private final Condition fieldWait; // cond. field

    private volatile Integer x;
    private volatile Integer y;

    private Coordinate current; // current instance being built

    private final static AtomicInteger assignedFieldNum = new AtomicInteger();
    private final static AtomicInteger readNum = new AtomicInteger();

    private CoordinateBuilder()
    {
        builderBusy = lock.newCondition();
        fieldWait = lock.newCondition();
    }

    public static CoordinateBuilder getBuilder()
    {
        return builder;
    }

    public Coordinate x(int aX) throws InterruptedException
    {
        lock.lock();

        try
        {
            while(x != null) // block other threads until current is built
            {
                builderBusy.await();
            }

            x = aX;

            if(assignedFieldNum.incrementAndGet() < 2) // wait until current.x and current.y are assigned
            {
                fieldWait.await();
            }
            else
            {
                current = new Coordinate(this);
                fieldWait.signalAll(); // instance built, wake up other builders
            }

            if(readNum.incrementAndGet() == 2) // reset state if all builders are about to return
            {
                x = null;
                y = null;
                assignedFieldNum.set(0);
                readNum.set(0);

                builderBusy.signalAll();
            }

            return current; 
        }
        finally
        {
            lock.unlock();
        }
    }

    public Coordinate y(int aY) throws InterruptedException
    {
        lock.lock();

        try
        {
            while(y != null) // block other threads until current is built
            {
                builderBusy.await();
            }

            y = aY;

            if(assignedFieldNum.incrementAndGet() < 2) // wait until current.x and current.y are assigned
            {
                fieldWait.await();
            }
            else
            {
                current = new Coordinate(this);
                fieldWait.signalAll(); // instance built, wake up other builders
            }

            if(readNum.incrementAndGet() == 2) // reset state if all builders are about to return
            {
                x = null;
                y = null;
                assignedFieldNum.set(0);
                readNum.set(0);

                builderBusy.signalAll();
            }

            return current; 
        }
        finally
        {
            lock.unlock();
        }
    }
}

CoordinateBuilder can be used from a thread as:

CoordinateBuilder builder = Coordinate.CoordinateBuilder.getBuilder();

// from Thread 1
Coordinate coord = builder.x(i);

// from another Thread 2
Coordinate coord = builder.y(j);

Thread 1 and Thread 2 assign different fields of the coord object and block until the coord object is built. As a result, Threads 1 and 2 get the same coord object. Any other threads calling builder methods in the meantime will block until the current instance is built.

I tested it but I am still not sure if this implementation is thread-safe. Do you see any defect with this approach?

\$\endgroup\$

migrated from stackoverflow.com Jan 6 '15 at 9:28

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ What is the purpose of CoordinateBuilder class? I cannot think of a situation where you would need something like this. \$\endgroup\$ – Banthar Jan 6 '15 at 10:12
  • \$\begingroup\$ See my edit with some explanations for its purpose and how it is used. \$\endgroup\$ – karakays Jan 6 '15 at 13:41
2
\$\begingroup\$

Your solution is very fragile because of next reasons:

  1. Threads which construct your object are depending on each other and blocks - this is bad - if something happens (interrupt/exception/bad scheduler event) and 2nd thread dies or not be able to complete - you will get deadlock. Even if this issue is avoidable with proper use of timeouts - this is still bad design to define these types of dependencies.

  2. The API you have is just setting some value on the builder object and this is not intuitive that this simple call is actually (OMG!) blocking call dependent on some random other thread.

To design it in better way - I would suggest not to block on your builder.x() method but instead return your builder immediately (instead of the object itself) and have object as partially constructed at this stage.

And then to have method like:

Coordinate waitCompleted(long timeout)

Which will block till object you need will be fully assembled - providing timeout to stop waiting if it will not complete in some reasonable time and make sure thread calls it can progress. This will give you better control from caller side on what is happening with the object you are constructing. And both your threads will have code like this

CoordinateBuilder builder = Coordinate.CoordinateBuilder.getBuilder();

// from Thread 1
CoordinateBuilder partial = builder.x(i);
Coordinale = partial.waitCompleted(1000);


// from another Thread 2
CoordinateBuilder partial = builder.y(i);
Coordinale = partial.waitCompleted(1000);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.