4
\$\begingroup\$

Background

I'm writing a wrapper for a significant portion of a native library in Java. The native objects my classes are wrapping are giving me some headaches in terms of design.

I'm not a fan of the two-step initialization idiom but I've resigned myself to use it if the native object has a lot of configuration options followed by a rather complex and error-prone initialization.

The real issue is that I still have some constructors that throw exceptions after native resources have been allocated, though I'm able to avoid it in most cases. All classes wrapping native resources implement the Disposable interface.

public interface Disposable extends AutoCloseable
{
   boolean isClosed();

   @Override
   void close();
}

The close method is always written in the most safest way, making no assumptions about the state of the object. Okay, so I can call it from the constructor... that is unless there are subclasses that override this method.

Code

So I've been thinking about an elegant solution to solve these problems. I've decided to upgrade the allocation and deallocation of native objects to full object status.

public interface Resource<T> extends Disposable
{
    T get();
}

These Resource objects are supposed to be pushed onto a stack that is shared with every class in the hierarchy so there is no longer a need to override the close method of a base class.

I have an abstract implementation that lazily allocates the native object once and returns it. I cannot use the double checked locking idiom to do this because I'm dealing with three states.

Either the native object hasn't been allocated, either it has, or it has been deallocated. If it has been deallocated, I don't want the possibility that it may accidentally be reallocated.

public abstract class AbstractResource<T> implements Resource<T>
{
    private enum State 
    {
        INITIAL,
        ALLOCATED,
        DISPOSED
    }

    private final AtomicReference<State> refState;
    private final AtomicReference<T> refResource; 

    public AbstractResource()
    {
        this.refState = new AtomicReference<>(State.INITIAL);
        this.refResource = new AtomicReference<>(null);
    }

    @Override
    public boolean isClosed() {
        return this.refState.get() == State.DISPOSED;
    }

    @Override
    public T get()
    {
        final State currentState = this.refState.get();
        T resource = this.refResource.get();

        if (currentState == State.DISPOSED)
            throw new ObjectDisposedException(resource.getClass().getName());

        if (currentState == State.INITIAL) {
            if (this.refState.compareAndSet(State.INITIAL, State.ALLOCATED)) {
                try {
                    resource = this.allocate();
                    this.refResource.set(resource);
                } catch (Exception ex) {
                    this.refState.set(State.INITIAL);
                    throw ex;
                }
            }
        }
        return resource;
    }

    protected abstract T allocate();

    @Override
    public void close() 
    {
        if (this.refState.getAndSet(State.DISPOSED) == State.ALLOCATED)
            this.release(this.refResource.get());
    }

    protected abstract void release(final T resource);
}

Is this class 100% thread-safe or have I missed anything? Might there be a better way to do this that does not require locking or at least keep it to a minimum?

\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. Good job on your first post! \$\endgroup\$ – TheCoffeeCup Nov 17 '15 at 2:16
2
\$\begingroup\$

I'm not a fan of the two-step initialization idiom but I've resigned myself to use it if the native object has a lot of configuration options followed by a rather complex and error-prone initialization.

The real issue is that I still have some constructors that throw exceptions after native resources have been allocated, though I'm able to avoid it in most cases.

That's a suspicious set of problems to have; too bad you didn't ask for a review of that code. Off the top of my head, however, I've generally found that "a lot of configuration options" can usually be managed by a builder, and that trivial constructors seldom throw -- dependency injection pushes the exception risk outside of the constructor, where you can usually manage things more easily.

@Override
public void close() 

If you aren't intending to let people create sub classes that circumvent the state machine, you might make these methods final.

protected abstract T allocate();
protected abstract void release(final T resource);

These methods don't belong here. The rest of your class is about lifecycle management; these are about resource management. You've mixed the concern about coordinating the life cycle with your concern for creation.

interface ResourceFactory<T> {
    void T allocate();
    void release(T resource);
}

Then, in your lifecycle management, defer the work to the factory.

@Override
public void close() 
{
    if (this.refState.getAndSet(State.DISPOSED) == State.ALLOCATED)
        this.factory.release(this.resource);
}

10 out of 10 for making the state machine explicit. I suspect that you should make State.ALLOCATING and State.DISPOSING explicit. You might also want to look into Stateless4j; trivial state machines often grow in complexity.

\$\endgroup\$
  • \$\begingroup\$ There are only ever three states that occur sequentially: configure -> open/initialize -> close. So I'm not worried about it growing in complexity. I considered a builder for some time but if it takes care of the configuration step, then we can initialize. I don't think the builder should be responsible for it and neither should the constructor because it usually involves IO and is error-prone. If I put it in a separate method, then we're come back to the two-step idiom yet again. I believe it is best to model the native API in this case, however much I may hate it, unless there's another way. \$\endgroup\$ – Hachiman Nov 18 '15 at 10:23
  • \$\begingroup\$ I'll come right out and say that it is the FFmpeg libraries that I'm wrapping. If you're familiar with it, you'll know exactly why this is challenging. Why not use existing projects you ask? Most of the them are CLI wrappers which do not offer all the functionality I require, others are outdated like Xuggler and others are riddled with bugs and false assumptions. I'd very much like to post some code for review but I'm not entirely sure if it's too specific for this site or not. \$\endgroup\$ – Hachiman Nov 18 '15 at 10:49
  • 1
    \$\begingroup\$ @Hachiman Go ahead and post it. Nobody's ever gotten a question closed for being too specific. :-) Worst case is you get no answers, but that doesn't happen much for good questions. \$\endgroup\$ – Eric Stein Nov 18 '15 at 14:37
1
\$\begingroup\$

Okay, after sleeping on it, there is clearly a race condition. If thread A is busy allocating the object, then a concurrent thread B would read a null that is still stored in refResource. Obviously, there must be a lock. What was I thinking!

I ended up using the two-field variant of the double-checked locking idiom that Guava seems to be using in their MemoizingSupplier implementation. Now, I can keep using my refState variable safely with the DCL idiom. The code also looks a bit cleaner now.

public abstract class AbstractResource<T> implements Resource<T>
{
    private enum State 
    {
        INITIAL,
        ALLOCATED,
        DISPOSED
    }

    private final AtomicReference<State> refState;
    private T resource; 

    public AbstractResource()
    {
        this.refState = new AtomicReference<>(State.INITIAL);
        this.resource = null;
    }

    @Override
    public boolean isClosed() {
        return this.refState.get() == State.DISPOSED;
    }

    @Override
    public T get()
    {
        State lastState = this.refState.get();

        if (lastState == State.DISPOSED)
            throw new ObjectDisposedException(resource.getClass().getName());

        if (lastState == State.INITIAL) {
            synchronized(this.refState) {
                lastState = this.refState.get();
                if (lastState == State.INITIAL) {
                    T result = this.allocate();
                    this.resource = result;
                    this.refState.set(State.ALLOCATED);
                }
            }
        }
        return resource;
    }

    protected abstract T allocate();

    @Override
    public void close() 
    {
        if (this.refState.getAndSet(State.DISPOSED) == State.ALLOCATED)
            this.release(this.resource);
    }

    protected abstract void release(final T resource);
}

I was initially confused as to why resource was nonvolatile in Guava's implementation. I've since learned some interesting properties of volatile reads and writes and how it affects cache coherence.

As I understand it, resource can be nonvolatile and visible to all threads if and only if it is updated before a volatile write of refState and it is read after a volatile read of refState.

\$\endgroup\$
  • \$\begingroup\$ It seems I have to wait another 15 hours. Thanks, I didn't know I could. \$\endgroup\$ – Hachiman Nov 18 '15 at 9:58

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.