Re: JavaHL: JNIError: bad C++ this

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.AbstractJhlClientAdapter.dispose(AbstractJhlClientAdapter.java:3007)
>>> at
>>>
>>> org.tigris.subversion.subclipse.core.SVNClientManager.returnSVNClient(SVNClientManager.java:162)
>>> at
>>>
>>> org.tigris.subversion.subclipse.ui.operations.CommitOperation.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$ResourceJob.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.CommitOperation.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.

Thoughts ?

---
Walter Klust