Deleting jobject references in object assignment JNI invocation API

Question

I want to perform the following loop:

for(absolute_date CURRENT_DATE = START_DATE; CURRENT_DATE.COMPARE_TO(END_DATE) <= 0; CURRENT_DATE.SHIFT_SELF(TIMESTEP))
{
    CURRENT_STATE = Propagator.PROPAGATE(CURRENT_DATE);
}

Where CURRENT_STATE is an object of class state. propagator::PROPAGATE() is a method that returns an object of class state.

My state class is effectively a class wrapper for a Java library class which i'm calling via JNI invocation API. The problem I'm having is that I want to delete the local java references withDeleteLocalRefto prevent memory leaks (especially important as I will be looping many thousands of times).

However, since DeleteLocalRef is called in my state class destructor, the reference to the java jobject is destroyed as the RHS of the assignment returns, thus making CURRENT_STATE invalid as it contains a reference to a jobject which has been deleted.

How do I avoid this?

@Wheezil

Regarding your first point - since I am using the invocation API i.e. creating a virtual machine inside C++ and calling Java functions, I don't think I need to convert to Global References (as all local references remain valid until the JVM is destroyed or until a thread is detached). In this case I am not detaching and re-attaching threads to the JVM so the local references never get deleted. The important thing for me is to make sure the local references get deleted within JVM.

Regarding second point- I have already inhibited copying by setting my copy constructors/assignment operators = delete. My issue is more specifically about how to ensure these references get deleted.

My state class looks like this:

state::state(JNIEnv* ENV)
{
    this->ENV = ENV;
    this->jclass_state = ENV->FindClass("/path/to/class");
    this->jobject_state = nullptr;                                                  
}

state::~state()
{
    if(DOES_JVM_EXIST())
    {
        ENV->DeleteLocalRef(this->jclass_state); 
        ENV->DeleteLocalRef(this->jobject_state); //PROBLEMATIC
    }
}

state::state(state&& state_to_move)
{
    this->ENV = state_to_move.ENV;

    //move jobjects from mover => new object
    this->jobject_state = state_to_move.jobject_state;
    this->jclass_state = state_to_move.jclass_state;
}

state& state::operator =(state&& state_to_move)
{
    this->ENV = state_to_move.ENV;

    //move jobjects from mover => current object
    this->jobject_state= state_to_move.jobject_state;
    this->jclass_state = state_to_move.jclass_state;
    return *this;
} 

TO describe the problem I'm facing in more detail: the propagator::PROPAGATE() method returns a state object by value (currently stack allocated). As soon as this function returns, the following happens:

1) The move assignment operator is invoked. This sets the jobject_state and jclass_state members in the CURRENT_STATE object.

2) The destructor is invoked for the state instance created in the PROPAGATE() function. This deletes the local reference to jobject_state and thus the CURRENT_STATE Object no longer has a valid member variable.

Answer 1

Where to start... JNI is incredibly finicky and unforgiving, and if you don't get things just right, it will blow up. Your description is rather thin (please provide more detail if this doesn't help), but I can make a good guess. There are several problems with your approach. You are presumably doing something like this:

struct state {
   state(jobject thing_) : thing(thing_) {}
   ~state() { env->DeleteLocalRef(thing); }
   jobject thing;
}

The first issue is that storing local refs is dangerous. You cannot hang onto them beyond the current JNI frame. So convert them to global:

struct state {
   state(jobject thing_) : thing(env->NewGlobalRef(thing_)) { 
      env->DeleteLocaLRef(thing_); 
   }
   ~state() { env->DeleteGlobalRef(thing); }
   jobject thing;
}

The second issue is that jobject is basically like the old C++ auto_ptr<> -- really unsafe, because copying it leads to danging pointers and double-frees. So you either need to disallow copying of state and maybe only pass around state*, or make a copy-constructor that works:

state(const state& rhs) thing(env->NewGlobalRef(rhs.thing)) {}

This should at least get you on the right track.

UPDATE: Ddor, regarding local vs global references, this link describes it well: "Local references become invalid when the execution returns from the native method in which the local reference is created. Therefore, a native method must not store away a local reference and expect to reuse it in subsequent invocations." You can keep local references, but only under strict circumstances. Note that in particular, you cannot hand these to another thread, which it seems you are not doing. Another thing -- there are limits on the total number of local references that can be active. This limit is frustratingly underspecified, but it seems JVM-specific. I advise caution and always convert to global.

I thought that I had read somewhere, that you do not need to delete jclass because FindClass() always returns the same thing, but I'm having a hard time verifying this. In our code, we always convert the jclass to a global reference as well.

ENV->DeleteLocalRef(this->jclass_state); 

I must admit ignorance about C++ move semantics; just make sure that the default copy ctor is not called and your jobject_state is not freed twice.

this->jobject_state = state_to_move.jobject_state;

If your move constructor is being called instead of the copy constructor or assignment, I don't know why you would be seeing a delete on the destruction of the temporary. As I said, I am not expert on move semantics. I've always had the copy-constructor create a new global. reference.

Answer 2

You can not do this:

this->ENV = ENV;

You are caching the JNIEnv value passed to native code from the JVM.

You can't do that.

OK, to be pedantic, there are some instances where you can, but that can only work from a single thread, so there's no need to cache a JNIEnv * value for later reference when it's used by the same thread.

From the complexity of what you've posted, I seriously doubt you can guarantee your native code gets called by the same thread every single time.

You get passed a JNIenv * every time native code gets called from the JVM, so there's pretty much never any point in caching the JNIEnv value.

IMO, you are making your native code way too complex. There's no need to be caching and tracking all those references. It looks like you're trying to keep a native C++ object synchronized with a Java object. Why? If native code needs access to a Java object, simply pass that object in the call from Java to the native code.