[jira] [Comment Edited] (MINIFICPP-751) Discussion: RAII over JNI UTF strings

["Mr TheSegfault (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MINIFICPP-751?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16781940#comment-16781940 ] 

Mr TheSegfault edited comment on MINIFICPP-751 at 3/1/19 6:07 PM:
------------------------------------------------------------------

My opinion is that we are implementing code that should be implemented properly by the JNI consumer. In the case of JNI interactions, we can use xcheck to verify that data isn't lost and problems don't arise. In this case it may help us identify memory leaks. So the "error" in doing what we're currently doing case is not telling the JVM to deallocate a character reference to its internal data. Once the data is removed ( since it's a local reference to a jstring ) you will have a stale reference we can't use and potentially a pointer's worth of memory leaked. Again, this is the "error" in this  case.

By using RAII, we have the potential that the confines of the comments are broken and irrecoverable errors and failures in the VM OR non-deterministic behavior. 

I favor correcting any leaks with xcheck ( part of testing ) versus using RAII and opening the door for errors that may cause complete failure or worse, non-deterministic errors. I specifically did not choose this the route of wrapping strings and other objects since the lifetime of objects is something that can cause major issues that aren't always easily detectable.

 

Further, the environment is not something that you own. We would likely have to keep references to the "JavaServicer" as it were and attach the thread to the environment. if anyone takes a pointer or allocates one of these and happens to access it outside of the creator's thread without attaching it will lead to other failures not already discussed.

 

Finally, since we'd likely expose the underlying data ( const char *). We can't assume the caller isn't going to take that ref and use it elsewhere. Once that ref is removed, the local ref decremented, and GC executed that reference will be cause a SIGSEV by the JVM. 

As a result I'm not in favor of wrapping jstrings unless they are global references. In this case you add references we don't already have, increase GC pressure, and potentially impact performance. We control global reference lifetimes, so the same checks above for memory leaks in the JVM need to be performed, resulting in little to no lost benefit. I would argue that modifiers of this extension need to know JVM internals, so accepting commits will always require checks that I don't think this JNI String container would alleviate. 


was (Author: phrocker):
My opinion is that we are implementing code that should be implemented properly by the JNI consumer. In the case of JNI interactions, we can use xcheck to verify that data isn't lost and problems don't arise. In this case it may help us identify memory leaks. So the "error" in this case is not telling the JVM to deallocate a character reference to its internal data. Once the data is removed ( since it's a local reference to a jstring ) you will have a stale reference we can't use and potentially a pointer's worth of memory leaked. Again, this is the "error" in this  case.

By using RAII, we have the potential that the confines of the comments are broken and irrecoverable errors and failures in the VM OR non-deterministic behavior. 

I favor correcting any leaks with xcheck ( part of testing ) versus using RAII and opening the door for errors that may cause complete failure or worse, non-deterministic errors. I specifically did not choose this the route of wrapping strings and other objects since the lifetime of objects is something that can cause major issues that aren't always easily detectable.

 

Further, the environment is not something that you own. We would likely have to keep references to the "JavaServicer" as it were and attach the thread to the environment. if anyone takes a pointer or allocates one of these and happens to access it outside of the creator's thread without attaching it will lead to other failures not already discussed.

 

Finally, since we'd likely expose the underlying data ( const char *). We can't assume the caller isn't going to take that ref and use it elsewhere. Once that ref is removed, the local ref decremented, and GC executed that reference will be cause a SIGSEV by the JVM. 

As a result I'm not in favor of wrapping jstrings unless they are global references. In this case you add references we don't already have, increase GC pressure, and potentially impact performance. We control global reference lifetimes, so the same checks above for memory leaks in the JVM need to be performed, resulting in little to no lost benefit. I would argue that modifiers of this extension need to know JVM internals, so accepting commits will always require checks that I don't think this JNI String container would alleviate. 

> Discussion: RAII over JNI UTF strings
> -------------------------------------
>
>                 Key: MINIFICPP-751
>                 URL: https://issues.apache.org/jira/browse/MINIFICPP-751
>             Project: NiFi MiNiFi C++
>          Issue Type: Improvement
>            Reporter: Arpad Boda
>            Assignee: Arpad Boda
>            Priority: Minor
>
> In this PR #489 I've noticed that JNI UTF string usage is error-prone as releasing can be easily missed by the developer or simply get skipped because of an exception. 
> To avoid this my idea was to create a wrapper object that can be used as an std::string and handles get/release calls.
> As [~phrocker] pointed out, this object has to be kept in a small block to avoid storing ref on both the string itself and the JNI env. 
> This can partly be achieved by blocking copy/move construction/assigment and new, but still leaves some possibility to allocate this object on heap. 
> In my opinion the restrictions above with some comments in the class would help and make it safer, although I'm not sure it worth the effort.
>  
> [~phrocker], [~aldrin], what's your opinion?



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)