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