0

Long time Java, first time C++.

I'm writing a Java class that wraps an existing C++ API and looks something like this:

public class Foo implements Closeable {

    private long handle;

    public Foo(File file) {
        this.handle = Foo.openFile(file.toString());
    }

    // other methods go here...

    public void close() {
        Foo.closeFile(this.handle);
        this.handle = 0;
    }

    private static native long openFile(String file);
    private static native void closeFile(long handle);
}

We will stuff the pointer to the native C++ object into the handle field.

The problem is that the C++ library doesn't give me a pointer, it gives me a unique_ptr<Foo>. So this has to be moved onto the heap in openFile() (so it doesn't go out of scope), and then destroyed in closeFile().

I'm having trouble getting this to work, and am not sure if I'm doing it properly. What is the simplest/cleanest way to do this correctly in C++?

Here's what I'm currently doing:

// This holds a unique_ptr<T>, typically on the heap, until no longer needed
template <class T>
class PointerHolder {
public:
    PointerHolder(unique_ptr<T> ptr) {
        this->ptr = std::move(ptr);
    }
    T * get() const {
        return this->ptr.get();
    }
private:
    unique_ptr<T> ptr;
};

// Holder for a Foo
typedef PointerHolder<Foo> FooHolder;

jlong JNICALL
Java_Foo_openFile(JNIEnv *jenv, jclass jcls, jstring file)
{
    ...

    // Get smart pointer to native object from provided C++ library
    unique_ptr<Foo> ptr = ...

    // Save pointer on the heap until closeFile() is invoked sometime later
    const FooHolder *const holder = new FooHolder(std::move(ptr));

    // Return pointer encoded as a Java long
    return reinterpret_cast<jlong>(holder);
}

void JNICALL
Java_Foo_closeFile(JNIEnv *jenv, jclass jcls, jlong handle)
{
    // Delete pointer holder, and by extension, storage object
    delete reinterpret_cast<FooHolder *>(handle);        // CRASH HERE
}

Is this correct? If so, can it be simplified?

In any case, right now the program is crashing in closeFile(), but I can't tell if it's crashing because I'm doing something wrong or there is some bug in the library I'm using (it's relatively new and buggy).

Thanks for any "pointers".

Archie
  • 4,680
  • 1
  • 26
  • 35
  • You should check if ``handle`` is nil or not before deleting or not – Asesh Jan 22 '19 at 04:16
  • 3
    @Asesh it's legal (and a harmless no-op) to call delete on a NULL pointer in C++. If the delete line is crashing, it's because its argument is a non-NULL but also invalid pointer. – Jeremy Friesner Jan 22 '19 at 04:26
  • 1
    An owning pointer to a `std::unique_ptr` is basically the same as not having a `std::unique_ptr` and just using a *raw pointer*. – Galik Jan 22 '19 at 05:34

1 Answers1

5

It can be simplified.

Since you have to integrate with Java's lifetime management, unique_ptr isn't doing you any favors. You might as well manage the lifetime of the T object directly, instead of a wrapper for it (and certainly not a FooHolder wrapper for a unique_ptr wrapper for a T -- that's way too much wrapping).

Just call release() on the unique_ptr<T> to get a T* that needs to be delete-d when you are done with it.

Ben Voigt
  • 260,885
  • 36
  • 380
  • 671
  • So what I have is not optimal, but correct, so there must be a bug in the library - right? Also, isn't it possible the `unique_prtr` has a custom deleter that should be used, which would get lost if we just `release()` the pointer and then invoke regular `delete ptr` later? – Archie Jan 22 '19 at 14:21
  • @Archie: Unlike `shared_ptr` which can hold an arbitrary custom deleter, in `unique_ptr` deleter customization is part of the type. If you aren't being given a `unique_ptr` then you have a default deleter. – Ben Voigt Jan 22 '19 at 14:42
  • @Archie: And the bug could be in the consuming Java code, for example if it calls `close()` while another thread is actively using the object. I don't see anything wrong with the wrapper, though. – Ben Voigt Jan 22 '19 at 14:47