Better wrapping structure.

Avoid the Handle and directly use a shared_ptr.
This simplify a lot the wrapping code and potentially remove some bug.
This commit is contained in:
Matthieu Gautier 2023-01-11 16:15:08 +01:00
parent 030b2f95f5
commit 7887202eb0
9 changed files with 115 additions and 139 deletions

View File

@ -31,20 +31,19 @@
#include <zim/item.h>
/* Kiwix Reader JNI functions */
JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchive(
JNIEnv* env, jobject obj, jstring filename)
JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchive(
JNIEnv* env, jobject thisObj, jstring filename)
{
std::string cPath = TO_C(filename);
LOG("Attempting to create reader with: %s", cPath.c_str());
Lock l;
try {
zim::Archive* reader = new zim::Archive(cPath);
return reinterpret_cast<jlong>(new Handle<zim::Archive>(reader));
auto archive = std::make_shared<zim::Archive>(cPath);
SET_PTR(archive);
} catch (std::exception& e) {
LOG("Error opening ZIM file");
LOG("%s", e.what());
return 0;
}
}
@ -68,8 +67,8 @@ int jni2fd(const jobject& fdObj, JNIEnv* env)
} // unnamed namespace
JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveByFD(
JNIEnv* env, jobject obj, jobject fdObj)
JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchiveByFD(
JNIEnv* env, jobject thisObj, jobject fdObj)
{
#ifndef _WIN32
int fd = jni2fd(fdObj, env);
@ -77,22 +76,20 @@ JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveByFD(
LOG("Attempting to create reader with fd: %d", fd);
Lock l;
try {
zim::Archive* reader = new zim::Archive(fd);
return reinterpret_cast<jlong>(new Handle<zim::Archive>(reader));
auto archive = std::make_shared<zim::Archive>(fd);
SET_PTR(archive);
} catch (std::exception& e) {
LOG("Error opening ZIM file");
LOG("%s", e.what());
return 0;
}
#else
jclass exception = env->FindClass("java/lang/UnsupportedOperationException");
env->ThrowNew(exception, "org.kiwix.libzim.Archive.getNativeArchiveByFD() is not supported under Windows");
return 0;
env->ThrowNew(exception, "org.kiwix.libzim.Archive.setNativeArchiveByFD() is not supported under Windows");
#endif
}
JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveEmbedded(
JNIEnv* env, jobject obj, jobject fdObj, jlong offset, jlong size)
JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchiveEmbedded(
JNIEnv* env, jobject thisObj, jobject fdObj, jlong offset, jlong size)
{
#ifndef _WIN32
int fd = jni2fd(fdObj, env);
@ -100,17 +97,15 @@ JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveEmbedded(
LOG("Attempting to create reader with fd: %d", fd);
Lock l;
try {
zim::Archive* reader = new zim::Archive(fd, offset, size);
return reinterpret_cast<jlong>(new Handle<zim::Archive>(reader));
auto archive = std::make_shared<zim::Archive>(fd, offset, size);
SET_PTR(archive);
} catch (std::exception& e) {
LOG("Error opening ZIM file");
LOG("%s", e.what());
return 0;
}
#else
jclass exception = env->FindClass("java/lang/UnsupportedOperationException");
env->ThrowNew(exception, "org.kiwix.libzim.Archive.getNativeArchiveEmbedded() is not supported under Windows");
return 0;
env->ThrowNew(exception, "org.kiwix.libzim.Archive.setNativeArchiveEmbedded() is not supported under Windows");
#endif
}
@ -120,7 +115,7 @@ Java_org_kiwix_libzim_Archive_dispose(JNIEnv* env, jobject thisObj)
dispose<zim::Archive>(env, thisObj);
}
#define THIS (Handle<zim::Archive>::getHandle(env, thisObj))
#define THIS GET_PTR(zim::Archive)
#define GETTER(retType, name) JNIEXPORT retType JNICALL \
Java_org_kiwix_libzim_Archive_##name (JNIEnv* env, jobject thisObj) \
{ \
@ -144,16 +139,16 @@ METHOD(jstring, Archive, getMetadata, jstring name) {
}
METHOD(jobject, Archive, getMetadataItem, jstring name) {
auto item = THIS->getMetadataItem(TO_C(name));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item);
auto obj = NEW_OBJECT("org/kiwix/libzim/Item");
SET_HANDLE(zim::Item, obj, THIS->getMetadataItem(TO_C(name)));
return obj;
}
GETTER(jobjectArray, getMetadataKeys)
METHOD(jobject, Archive, getIllustrationItem, jint size) {
auto item = THIS->getIllustrationItem(TO_C(size));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item);
auto obj = NEW_OBJECT("org/kiwix/libzim/Item");
SET_HANDLE(zim::Item, obj, THIS->getIllustrationItem(TO_C(size)));
return obj;
}
@ -164,44 +159,44 @@ METHOD(jboolean, Archive, hasIllustration, jint size) {
GETTER(jlongArray, getIllustrationSizes)
METHOD(jobject, Archive, getEntryByPath, jlong index) {
auto entry = THIS->getEntryByPath(TO_C(index));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getEntryByPath(TO_C(index)));
return obj;
}
METHOD(jobject, Archive, getEntryByPath, jstring path) {
auto entry = THIS->getEntryByPath(TO_C(path));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getEntryByPath(TO_C(path)));
return obj;
}
METHOD(jobject, Archive, getEntryByTitle, jlong index) {
auto entry = THIS->getEntryByTitle(TO_C(index));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getEntryByTitle(TO_C(index)));
return obj;
}
METHOD(jobject, Archive, getEntryByTitle, jstring title) {
auto entry = THIS->getEntryByTitle(TO_C(title));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getEntryByTitle(TO_C(title)));
return obj;
}
METHOD(jobject, Archive, getEntryByClusterOrder, jlong index) {
auto entry = THIS->getEntryByClusterOrder(TO_C(index));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getEntryByClusterOrder(TO_C(index)));
return obj;
}
METHOD0(jobject, Archive, getMainEntry) {
auto entry = THIS->getMainEntry();
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getMainEntry());
return obj;
}
METHOD0(jobject, Archive, getRandomEntry) {
auto entry = THIS->getRandomEntry();
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getRandomEntry());
return obj;
}

View File

@ -28,13 +28,14 @@
#include <zim/blob.h>
#define NATIVE_TYPE zim::Blob
JNIEXPORT void JNICALL
Java_org_kiwix_kiwixlib_libzim_Blob_dispose(JNIEnv* env, jobject thisObj)
{
dispose<zim::Blob>(env, thisObj);
dispose<NATIVE_TYPE>(env, thisObj);
}
#define THIS (Handle<zim::Blob>::getHandle(env, thisObj))
#define THIS GET_PTR(NATIVE_TYPE)
#define GETTER(retType, name) JNIEXPORT retType JNICALL \
Java_org_kiwix_libzim_Blob__##name (JNIEnv* env, jobject thisObj) \
{ \
@ -42,6 +43,6 @@ Java_org_kiwix_libzim_Blob__##name (JNIEnv* env, jobject thisObj) \
}
METHOD0(jstring, Blob, getData) {
return TO_JNI(std::string(**THIS));
return TO_JNI(std::string(*THIS));
}
GETTER(jlong, size)

View File

@ -34,10 +34,10 @@
JNIEXPORT void JNICALL
Java_org_kiwix_kiwixlib_libzim_Entry_dispose(JNIEnv* env, jobject thisObj)
{
dispose<zim::Entry>(env, thisObj);
dispose<NATIVE_TYPE>(env, thisObj);
}
#define THIS (Handle<zim::Entry>::getHandle(env, thisObj))
#define THIS GET_PTR(NATIVE_TYPE)
#define GETTER(retType, name) JNIEXPORT retType JNICALL \
Java_org_kiwix_libzim_Entry__##name (JNIEnv* env, jobject thisObj) \
{ \
@ -49,19 +49,19 @@ GETTER(jboolean, isRedirect)
GETTER(jstring, getTitle)
GETTER(jstring, getPath)
METHOD(jobject, Entry, getItem, jboolean follow) {
auto item = THIS->getItem(TO_C(follow));
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item);
auto obj = NEW_OBJECT("org/kiwix/libzim/Item");
SET_HANDLE(zim::Item, obj, THIS->getItem(TO_C(follow)));
return obj;
}
METHOD0(jobject, Entry, getRedirect) {
auto item = THIS->getRedirect();
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item);
auto obj = NEW_OBJECT("org/kiwix/libzim/Item");
SET_HANDLE(zim::Item, obj, THIS->getRedirect());
return obj;
}
METHOD0(jobject, Entry, getRedirectEntry) {
auto entry = THIS->getRedirectEntry();
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry);
auto obj = NEW_OBJECT("org/kiwix/libzim/Entry");
SET_HANDLE(zim::Entry, obj, THIS->getRedirectEntry());
return obj;
}

View File

@ -28,13 +28,15 @@
#include <zim/item.h>
#define NATIVE_TYPE zim::Item
JNIEXPORT void JNICALL
Java_org_kiwix_kiwixlib_libzim_Item_dispose(JNIEnv* env, jobject thisObj)
{
dispose<zim::Item>(env, thisObj);
dispose<NATIVE_TYPE>(env, thisObj);
}
#define THIS (Handle<zim::Item>::getHandle(env, thisObj))
#define THIS GET_PTR(NATIVE_TYPE)
#define GETTER(retType, name) JNIEXPORT retType JNICALL \
Java_org_kiwix_libzim_Item__##name (JNIEnv* env, jobject thisObj) \
{ \
@ -46,8 +48,8 @@ GETTER(jstring, getPath)
GETTER(jstring, getMimetype)
METHOD0(jobject, Item, getData) {
auto blob = THIS->getData();
auto obj = CREATE_WRAPPER("org/kiwix/libzim/Blob", blob);
auto obj = NEW_OBJECT("org/kiwix/libzim/Blob");
SET_HANDLE(zim::Blob, obj, THIS->getData());
return obj;
}

View File

@ -38,34 +38,74 @@
#endif
extern std::mutex globalLock;
using std::shared_ptr;
// Here is the wrapping structure.
// All java class wrapping a `Native` instance must contain a pointer (cast as a long (J)) called "nativeHandle".
// They also need a constructor taking no argument (or they can, but helper here cannot be used.)
// The "nativeHandle" pointer points to a heap allocated `Handle<Native>`. The handle itself IS a shared_ptr.
// !!! This is a shared_ptr heap allocated instance which contains a `Native` heap allocated.
// So if the `Native` instance is own by a smart pointer (shared_ptr), the shared_ptr itself is "own" by a raw pointer.
// From the jobject and the pointer field ("nativeHandle"), we can access a shared_ptr (not a pointer to a shared_ptr).
// To avoid a memory leak, we must at java wrapper destruction (dispose), delete the heap shared_ptr.
// Create a java object using a default constructor. No field is set.
inline jobject newObject(const char* className, JNIEnv* env) {
jclass objClass = env->FindClass(className);
jmethodID initMethod = env->GetMethodID(objClass, "<init>", "()V");
jobject wrapper = env->NewObject(objClass, initMethod);
return wrapper;
}
#define NEW_OBJECT(CLASSNAME) newObject(CLASSNAME, env)
// Set the pointer to the wrapped object.
template<typename T>
void setPtr(JNIEnv* env, jobject thisObj, T* ptr)
inline void setPtr(JNIEnv* env, jobject thisObj, shared_ptr<T>&& ptr, const char* handleName = "nativeHandle")
{
jclass thisClass = env->GetObjectClass(thisObj);
jfieldID fieldId = env->GetFieldID(thisClass, "nativeHandle", "J");
env->SetLongField(thisObj, fieldId, reinterpret_cast<jlong>(ptr));
jfieldID fieldId = env->GetFieldID(thisClass, handleName, "J");
// if field id is not null, we are leaking the currently stored handle
assert(env->GetLongField(thisObj, fieldId) == 0);
// allocate a shared_ptr on the head
shared_ptr<T>* handle = new shared_ptr<T>(ptr);
env->SetLongField(thisObj, fieldId, reinterpret_cast<jlong>(handle));
}
#define SET_PTR(SHARED_PTR) setPtr(env, thisObj, std::move(SHARED_PTR))
// This create a object and set it in the wrapper
template<typename T, typename ...Args>
void allocate(JNIEnv* env, jobject thisObj, Args && ...args)
inline void setHandle(JNIEnv* env, jobject thisObj, Args && ...args)
{
T* ptr = new T(std::forward<Args>(args)...);
setPtr(env, thisObj, ptr);
auto ptr = std::make_shared<T>(std::forward<Args>(args)...);
setPtr(env, thisObj, std::move(ptr));
}
#define SET_HANDLE(NATIVE_TYPE, OBJ, VALUE) setHandle<NATIVE_TYPE>(env, OBJ, VALUE)
// Return a shared_ptr for the handle
template<typename T>
T* getPtr(JNIEnv* env, jobject thisObj)
shared_ptr<T> getPtr(JNIEnv* env, jobject thisObj, const char* handleName = "nativeHandle")
{
jclass thisClass = env->GetObjectClass(thisObj);
jfieldID fidNumber = env->GetFieldID(thisClass, "nativeHandle", "J");
return reinterpret_cast<T*>(env->GetLongField(thisObj, fidNumber));
jfieldID fidNumber = env->GetFieldID(thisClass, handleName, "J");
auto handle = reinterpret_cast<shared_ptr<T>*>(env->GetLongField(thisObj, fidNumber));
return *handle;
}
#define GET_PTR(NATIVE_TYPE) getPtr<NATIVE_TYPE>(env, thisObj)
// Delete the heap allocated handle
template<typename T>
void dispose(JNIEnv* env, jobject thisObj)
void dispose(JNIEnv* env, jobject thisObj, const char* handleName = "nativeHandle")
{
delete getPtr<T>(env, thisObj);
jclass thisClass = env->GetObjectClass(thisObj);
jfieldID fieldId = env->GetFieldID(thisClass, handleName, "J");
auto handle = reinterpret_cast<shared_ptr<T>*>(env->GetLongField(thisObj, fieldId));
if (handle != 0) {
delete handle;
}
env->SetLongField(thisObj, fieldId, 0);
}
#define METHOD0(retType, class, name) \
@ -76,11 +116,11 @@ JNIEXPORT retType JNICALL Java_org_kiwix_libzim_##class##_##name( \
JNIEXPORT retType JNICALL Java_org_kiwix_libzim_##class##_##name( \
JNIEnv* env, jobject thisObj, __VA_ARGS__)
inline jfieldID getHandleField(JNIEnv* env, jobject obj)
inline jfieldID getHandleField(JNIEnv* env, jobject obj, const char* handleName)
{
jclass c = env->GetObjectClass(obj);
// J is the type signature for long:
return env->GetFieldID(c, "nativeHandle", "J");
return env->GetFieldID(c, handleName, "J");
}
inline jobjectArray createArray(JNIEnv* env, size_t length, const std::string& type_sig)
@ -98,55 +138,6 @@ class Lock : public std::unique_lock<std::mutex>
Lock() : std::unique_lock<std::mutex>(globalLock) { }
};
template <class T>
class LockedHandle;
/*
* A handle to a shared_ptr<T>
* Not usable by itself, you must get a LockedHandle to access the value.
*/
template <class T>
class Handle
{
protected:
std::shared_ptr<T> value;
public:
Handle(T* v) : value(v){};
// No destructor. This must and will be handled by dispose method.
static LockedHandle<T> getHandle(JNIEnv* env, jobject obj)
{
jlong handle = env->GetLongField(obj, getHandleField(env, obj));
return LockedHandle<T>(reinterpret_cast<Handle<T>*>(handle));
}
static void dispose(JNIEnv* env, jobject obj)
{
auto lHandle = getHandle(env, obj);
auto handle = lHandle.h;
delete handle;
}
friend class LockedHandle<T>;
};
/*
* A locked handle.
* Only one LockedHandle can exist at the same time.
* As LockedHandle is the
*/
template <class T>
struct LockedHandle : public Lock {
Handle<T>* h;
LockedHandle(Handle<T>* h) : h(h) {}
T* operator->() { return h->value.get(); }
T* operator*() { return h->value.get(); }
operator bool() const { return (h->value); }
operator T*() const { return h->value.get(); }
};
// ---------------------------------------------------------------------------
// Convert things to JAVA
// ---------------------------------------------------------------------------
@ -160,6 +151,8 @@ template<> struct JType<int64_t>{ typedef jlong type_t; };
template<> struct JType<uint64_t> { typedef jlong type_t; };
template<> struct JType<uint32_t> { typedef jlong type_t; };
template<> struct JType<std::string>{ typedef jstring type_t; };
template<> struct JType<float> { typedef jfloat type_t;};
template<> struct JType<double> { typedef jdouble type_t;};
template<typename T, typename U=T>
@ -282,6 +275,8 @@ template<> struct CType<jboolean>{ typedef bool type_t; };
template<> struct CType<jint>{ typedef int type_t; };
template<> struct CType<jlong>{ typedef long type_t; };
template<> struct CType<jstring>{ typedef std::string type_t; };
template<> struct CType<jfloat> { typedef float type_t; };
template<> struct CType<jdouble> { typedef double type_t; };
template<typename U>
struct CType<jobjectArray, U>{ typedef std::vector<typename CType<U>::type_t> type_t; };
@ -362,15 +357,4 @@ inline void setDaiObjValue(const std::string& filename, const long offset,
env->SetLongField(obj, offsetFid, offset);
}
template<typename T>
inline jobject createWrapper(const char* className, T && nativeObj, JNIEnv* env) {
jclass objClass = env->FindClass(className);
jmethodID initMethod = env->GetMethodID(objClass, "<init>", "(J)V");
jlong nativeHandle = reinterpret_cast<jlong>(new Handle<T>(new T(std::forward<T>(nativeObj))));
jobject object = env->NewObject(objClass, initMethod, nativeHandle);
return object;
}
#define CREATE_WRAPPER(CLASSNAME, OBJ) createWrapper(CLASSNAME, std::move(OBJ), env)
#endif // _ANDROID_JNI_UTILS_H

View File

@ -29,7 +29,7 @@ public class Archive
public Archive(String filename) throws ZimFileFormatException
{
nativeHandle = getNativeArchive(filename);
setNativeArchive(filename);
if (nativeHandle == 0) {
throw new ZimFileFormatException("Cannot open zimfile "+filename);
}
@ -37,7 +37,7 @@ public class Archive
public Archive(FileDescriptor fd) throws ZimFileFormatException
{
nativeHandle = getNativeArchiveByFD(fd);
setNativeArchiveByFD(fd);
if (nativeHandle == 0) {
throw new ZimFileFormatException("Cannot open zimfile by fd "+fd.toString());
}
@ -46,7 +46,7 @@ public class Archive
public Archive(FileDescriptor fd, long offset, long size)
throws ZimFileFormatException
{
nativeHandle = getNativeArchiveEmbedded(fd, offset, size);
setNativeArchiveEmbedded(fd, offset, size);
if (nativeHandle == 0) {
throw new ZimFileFormatException(String.format("Cannot open embedded zimfile (fd=%s, offset=%d, size=%d)", fd, offset, size));
}
@ -93,9 +93,9 @@ public class Archive
public native boolean hasNewNamespaceScheme();
private native long getNativeArchive(String filename);
private native long getNativeArchiveByFD(FileDescriptor fd);
private native long getNativeArchiveEmbedded(FileDescriptor fd, long offset, long size);
private native void setNativeArchive(String filename);
private native void setNativeArchiveByFD(FileDescriptor fd);
private native void setNativeArchiveEmbedded(FileDescriptor fd, long offset, long size);
///--------- The wrapper thing

View File

@ -23,8 +23,6 @@ import org.kiwix.libzim.Blob;
public class Blob
{
private Blob(long handle) {nativeHandle = handle;}
public native String getData();
public native long size();

View File

@ -23,8 +23,6 @@ import org.kiwix.libzim.Item;
public class Entry
{
private Entry(long handle) {nativeHandle = handle;}
public native boolean isRedirect();
public native String getTitle();
public native String getPath();

View File

@ -23,8 +23,6 @@ import org.kiwix.libzim.Blob;
public class Item
{
private Item(long handle) {nativeHandle = handle;}
public native String getTitle();
public native String getPath();
public native String getMimeType();