[ 
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)

Reply via email to