Re: JavaHL: JNIError: bad C++ this

On Tue, Aug 9, 2016 at 11:42 AM, Walter Klust <walter.klust_at_elego.de> wrote:

> On 08.07.2016 16:48, Walter Klust wrote:
>
>> On 08.07.2016 16:38, Mark Phippard wrote:
>>
>>> On Fri, Jul 8, 2016 at 4:58 AM, Walter Klust <walter.klust_at_elego.de>
>>> wrote:
>>>
>>> Hello,
>>>>
>>>> I have an interesting problem concerning JavaHL:
>>>>
>>>> - Windows 7, JRE 8u66, Eclipse 4.2.1, Subclipse 1.8.22, JavaHL 1.8.10
>>>> - a changeset of around 2000 files; roughly 50% modified and 50% deleted
>>>> (probably to a large refactoring)
>>>> - on trying to commit the changes eclipse pops up a message after a
>>>> short
>>>> time: "Internal error occured during SVNCommit", "bad C++ this"
>>>>
>>>>
>>> I do not know if you are using changeset generically or referencing the
>>> Eclipse changeset feature. But just to clarify for SVN devs, this is not
>>> the SVN changelist feature. In terms of SVN commit, it would just be a
>>> specific list of files to commit like any other commit. Eclipse
>>> changesets
>>> are just a UI feature to organize the modified files.
>>>
>>>
>> I meant a list of files to commit. Thanks for the clarification.
>>
>>
>>>
>>> - in the eclipse logs a stacktrace was found:
>>>>
>>>> org.apache.subversion.javahl.JNIError: bad C++ this
>>>> at org.apache.subversion.javahl.SVNClient.dispose(Native
>>>> Method)
>>>> at
>>>>
>>>> org.tigris.subversion.svnclientadapter.javahl.AbstractJhlCl
>>>> ientAdapter.dispose(AbstractJhlClientAdapter.java:3007)
>>>> at
>>>>
>>>> org.tigris.subversion.subclipse.core.SVNClientManager.retur
>>>> nSVNClient(SVNClientManager.java:162)
>>>> at
>>>>
>>>> org.tigris.subversion.subclipse.ui.operations.CommitOperati
>>>> on.execute(CommitOperation.java:113)
>>>> at
>>>>
>>>> org.tigris.subversion.subclipse.ui.operations.SVNOperation.
>>>> run(SVNOperation.java:90)
>>>> at
>>>>
>>>> org.eclipse.team.internal.ui.actions.JobRunnableContext.run
>>>> (JobRunnableContext.java:144)
>>>> at
>>>>
>>>> org.eclipse.team.internal.ui.actions.JobRunnableContext$Res
>>>> ourceJob.runInWorkspace(JobRunnableContext.java:72)
>>>> at
>>>>
>>>> org.eclipse.core.internal.resources.InternalWorkspaceJob.
>>>> run(InternalWorkspaceJob.java:38)
>>>> at org.eclipse.core.internal.jobs.Worker.run(Worker.java:53)
>>>>
>>>>
>>>> - this error does not occur when trying to commit a subset of the
>>>> changes,
>>>> e.g. any 2 of 4 subdirectories can be comitted successfully but not all
>>>> 4
>>>> together.
>>>>
>>>> It looks to me like a quantity problem; something like an internal
>>>> buffer
>>>> overflow due to the large changeset. I tried to reproduce this with a
>>>> similar number of modified/deleted files but had no success so far.
>>>>
>>>> Any ideas how to analyze this problem further ?
>>>>
>>>>
>>>> It sort of seems like it could be a Subclipse bug, though it is
>>> interesting
>>> that it only manifests in specific scenarios. Makes me not really sure
>>> where to look.
>>>
>>> Does the commit work or record any errors? This exception is only
>>> happening in some cleanup that occurs after the API calls would all be
>>> completed or possibly after having processed an exception. IOW, this is
>>> not failing during an API call, it is failing when Subclipse is just
>>> disposing of the JavaHL client it obtained to make the API calls.
>>> Something that is done for every API call.
>>>
>>> The stack trace does not make a lot of sense either. This should be the
>>> relevant place in the code:
>>>
>>> org.tigris.subversion.subclipse.ui.operations.CommitOperati
>>> on.execute(CommitOperation.java:113)
>>>
>>> But that does not line up with anything that makes a lot of sense for the
>>> error.
>>>
>>>
>>>
>> The commit does not work. Eclipse throws the message popup; the whole
>> commit process is then terminated.
>>
>> By the way: when changing to svnkit this problem does not occur. Not sure
>> what this means about the root cause.
>>
>>
>>
>>
> We analyzed the sources further:
>
> The JNIError can happen if there are multiple parallel dispose calls. Now
> there is code which should prevent this in a non thread safe environment:
>
> -------
>
> SVNClientManager.java
> package org.tigris.subversion.subclipse.core;
> ...
> public class SVNClientManager {
> ...
> public void returnSVNClient(ISVNClientAdapter client) {
> if (client == null || client.isThreadsafe())
> return;
> // For non-threadsafe clients we are done with the object
> so
> // let it clean up any resources it has allocated.
> client.dispose();
> client = null;
> }
>
> -------
>
> We think that this method is problematic with non-threadsafe clients.
> Assume there are two threads calling this method:
> - T1 enters, runs, executes client.dispose(). client is not null
> - T2 enters, runs, passes the test because client is not null and also
> executes client.dispose()
>
> This would lead to the C++-Exception.
>
>
> It should prevented in a non-threadsafe environment; the call
> client.isThreadsafe() is not sufficient.
>
>
> A fix for this would be to either declare the method returnSVNClient
> synchronized or to put the statements of the method into a monitored block.
>
>

The isThreadSafe() method is not part of JavaHL. It is in the Subclipse
adapter and it was originally just put in place to deal with an issue in
SVNKit where we could not call dispose method.

From what I see in current code, this is just false in both adapters which
means dispose is always called when we are done with it.

I do not believe this was ever specifically about thread safety. I do not
recall why I named the method that, but it was just a way to get different
runtime behavior when SVNKit was being used due to a difference in that
library.

-- 
Thanks
Mark Phippard
http://markphip.blogspot.com/