You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2005/11/14 18:00:45 UTC
r17214, JavaHL thread local storeage and reentrant calls
So there's a proposed backport in the 1.3.x STATUS file for revision
17214, which fixes some problems with reentrant calls into JavaHL
functions. I'm not a Java/JNI person at all, so bear with me while I
try to summarize what the problem was and what the fix is, so we can
all be on the same page when I talk about a potential problem.
Patrick, please feel free to correct me if I'm wrong on any of this,
since you obviously know more about Java than I do, and you did write
the code in question.
JavaHL uses thread local storeage to keep track of a variety of
things, for example it stores Java level exceptions there, so it's
possible to say "an exception was thrown" in some C code, and then
later it gets reported to the JVM. This is all stored in this
JNIThreadData object, which is created when we enter a JavaHL function
and destroyed when we leave it. Now the problem comes from when we
call a JavaHL function from another JavaHL function, there's only one
JNIThreadData object per thread, so they stomp all over each other.
The solution was to make the JNIThreadData object really a linked
list, so when we start the second call into a JavaHL function we just
push another JNIThreadData onto the stack, use it for this call, then
pop it off at the end leaving the old one there for the existing call
to pick up and use.
Now the problem becomes one of the mappings between the C level APR
calls JavaHL makes to manipulate the thread local storeage and the
Java level threads that are actually making the calls into JavaHL. If
a C level thread and a Java level thread map to the same "thing"
underneath, we can reasonably expect the C level thread local storeage
to work correctly, but if the JVM is doing something fancy, mapping
multiple Java level threads to a single OS level thread (a N-M
threading scheme, green threads, whatever you want to call it), then
it seems like this house of cards is likely to come crashing down.
So is this something we need to care about? I really don't know
enough about Java/JNI to say one way or the other, but it seems weird
enough to be concerned about.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
> I do not think it is at all unreasonable for a Java library to declare its
> thread-safety. I have seen several that flat-out say they are not thread
> safe. I do not see the problem with JavaHL providing a thread-safe and
> non thread-safe version of the interface. It is hardly a burden to change
> your code from:
>
> SVNClientInterface client = new SVNClient();
>
> to
>
> SVNClientInterface client = new SVNClientSynchronized();
>
> If you need thread safety.
That's fine, I have no problem with that, but if it is the case it
should at least be documented. Right now all there is in
SVNClientSynchronized is a javadoc comment saying this is a thread
safe wrapper for SVNClient, which to me implies "here is a version of
SVNClient you can instantiate and use from multiple threads", not "if
you have multiple threads using this library at all they better all go
through this class or it'll have the potential to break horribly".
> All this being said, it stinks that a fix for a JVM crash might not get
> into the next release candidate over issues that could have been brought
> up any time over the past couple of years. Yes, it would be great if
> Patrick were around and could vote for his own patches, but apparently he
> isn't. Is there no one else capable of reviewing and voting on these? In
> the end it is mostly just C++ code and is either valid or not.
It's not just C++ code, it's C++ and JNI and Java and the
interrelationships therin. If it was just C++ code I'd be able to
review it, as would other people in the project, but that's just not
the case for this change. The only other person I know of on this
project with any JNI experience is DLR, and he's not sure the patch is
100% right.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
JavaHL Thread Safety?
Posted by Conor MacNeill <co...@cenqua.com>.
Mark Phippard wrote:
>
> I do not think it is at all unreasonable for a Java library to declare its
> thread-safety. I have seen several that flat-out say they are not thread
> safe. I do not see the problem with JavaHL providing a thread-safe and
> non thread-safe version of the interface. It is hardly a burden to change
> your code from:
>
> SVNClientInterface client = new SVNClient();
>
> to
>
> SVNClientInterface client = new SVNClientSynchronized();
>
> If you need thread safety.
>
I guess I'd like to get a clarification here. As currently implemented
the SVNClientSynchronized implementation means there can only be a
single active SVNClient in the whole JVM. It's way more than "thread
safety".
Currently SVNClient says it is not "thread safe" and that's fine, but to
me that means I can only use one thread in a particular instance. It
does not suggest to me that I can really only use one instance at a time
in the whole VM.
A single active instance per VM seems a bit limiting to me. Could
someone please clarify that this is indeed the case.
Cheers
Conor
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Mark Phippard <ma...@softlanding.com>.
rooneg@gmail.com wrote on 11/14/2005 03:52:45 PM:
> On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
>
> > I do not believe that SVNClientSynchronized is synchronizing on a
specific
> > instance of the class. If it were, then it wouldn't be thread safe at
> > all. It is synchrnonizing on a static instance of the .class which I
> > believe will apply to all instances of the class and therefore all
> > threads.
> >
> > We use SVNClientSynchronized in Subclipse.
>
> Ok, but is it absolutely 100% required that any multithreaded code
> that uses any part of JavaHL go through the SVNClientSynchronized
> class? If so, that will prevent this problem, but if not it can still
> occur.
>
> If that is the case, btw, it seems rather odd to me. Why can't there
> be multiple instances of SVNClient doing totally unrelated things at
> the same time? There doesn't seem to be any fundamental reason that
> they should be using any shared state.
I do not think it is at all unreasonable for a Java library to declare its
thread-safety. I have seen several that flat-out say they are not thread
safe. I do not see the problem with JavaHL providing a thread-safe and
non thread-safe version of the interface. It is hardly a burden to change
your code from:
SVNClientInterface client = new SVNClient();
to
SVNClientInterface client = new SVNClientSynchronized();
If you need thread safety.
All this being said, it stinks that a fix for a JVM crash might not get
into the next release candidate over issues that could have been brought
up any time over the past couple of years. Yes, it would be great if
Patrick were around and could vote for his own patches, but apparently he
isn't. Is there no one else capable of reviewing and voting on these? In
the end it is mostly just C++ code and is either valid or not.
I can verify that the Java part of the code all looks good and also
Patrick added two tests to the test suite that test the re-entry
situation.
Finally, as I said before and know you are aware I can vouch that this
patch addresses the two JVM crash scenarios in Subclipse (which Patrick's
new tests also would exercise).
Our only workaround to these problems in Subclipse are to avoid any
re-entry situations. This means writing our own Java versions of
getAdminDirectory() and isAdminDirectory() (which I am fine with) and not
providing the UI option of cancelling an operation (which I wish we could
add).
Thanks
Mark
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
> I do not believe that SVNClientSynchronized is synchronizing on a specific
> instance of the class. If it were, then it wouldn't be thread safe at
> all. It is synchrnonizing on a static instance of the .class which I
> believe will apply to all instances of the class and therefore all
> threads.
>
> We use SVNClientSynchronized in Subclipse.
Ok, but is it absolutely 100% required that any multithreaded code
that uses any part of JavaHL go through the SVNClientSynchronized
class? If so, that will prevent this problem, but if not it can still
occur.
If that is the case, btw, it seems rather odd to me. Why can't there
be multiple instances of SVNClient doing totally unrelated things at
the same time? There doesn't seem to be any fundamental reason that
they should be using any shared state.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: r17214, JavaHL thread local storeage and reentrant calls
Posted by Alexander Kitaev <al...@tmate.org>.
> No, SVNClientSynchronized contains a static member variable
> that it synchronizes all access to SVNClient around. It just
> so happens that they use a Class object for the member
> variable. That is what I meant.
I understand now (after I looked at the code :)), thanks!
Alexander Kitaev,
TMate Software,
http://tmate.org/
http://jetbrains.com/tmate/
> -----Original Message-----
> From: Mark Phippard [mailto:markp@softlanding.com]
> Sent: Monday, November 14, 2005 22:11
> To: alex@tmate.org
> Cc: dev@subversion.tigris.org
> Subject: RE: r17214, JavaHL thread local storeage and reentrant calls
>
> "Alexander Kitaev" <al...@tmate.org> wrote on 11/14/2005 04:04:43 PM:
>
> > If you're talking about non-static synchronized java methods, then
> > non-static methods uses object monitor for synchronization,
> not class
> one:
>
> No, SVNClientSynchronized contains a static member variable
> that it synchronizes all access to SVNClient around. It just
> so happens that they use a Class object for the member
> variable. That is what I meant.
>
> I believe the technique that is being used in
> SVNClientSynchronized is fully thread-safe and is fairly common.
>
> Mark
>
>
>
> ______________________________________________________________
> _______________
> Scanned for SoftLanding Systems, Inc. and SoftLanding Europe
> Plc by IBM Email Security Management Services powered by MessageLabs.
> ______________________________________________________________
> _______________
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: r17214, JavaHL thread local storeage and reentrant calls
Posted by Mark Phippard <ma...@softlanding.com>.
"Alexander Kitaev" <al...@tmate.org> wrote on 11/14/2005 04:04:43 PM:
> If you're talking about non-static synchronized java methods, then
> non-static methods uses object monitor for synchronization, not class
one:
No, SVNClientSynchronized contains a static member variable that it
synchronizes all access to SVNClient around. It just so happens that they
use a Class object for the member variable. That is what I meant.
I believe the technique that is being used in SVNClientSynchronized is
fully thread-safe and is fairly common.
Mark
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: r17214, JavaHL thread local storeage and reentrant calls
Posted by Alexander Kitaev <al...@tmate.org>.
Hello Mark,
> I do not believe that SVNClientSynchronized is synchronizing
> on a specific instance of the class. If it were, then it
> wouldn't be thread safe at all. It is synchrnonizing on a
> static instance of the .class which I believe will apply to
> all instances of the class and therefore all threads.
If you're talking about non-static synchronized java methods, then
non-static methods uses object monitor for synchronization, not class one:
(from Java langauge spec.
http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.4.3.6):
A synchronized method acquires a monitor (ยง17.1) before it executes. For a
class (static) method, the monitor associated with the Class object for the
method's class is used. For an instance method, the monitor associated with
this (the object for which the method was invoked) is used.
...
Actullay making non-static method synchronized is the same as writing:
public void test() {
synchronized(this) {....}
}
To make sure that all calls to SVNClient object are synchronized you should
use a single instance of SVNClient per application or per project, so that
it will be possible to run several parallel task that work with unrelated
working copies.
Alexander Kitaev,
TMate Software,
http://tmate.org/
http://jetbrains.com/tmate/
> -----Original Message-----
> From: Mark Phippard [mailto:markp@softlanding.com]
> Sent: Monday, November 14, 2005 21:41
> To: Garrett Rooney
> Cc: ajv-lists@netspace.net.au; dev@subversion.tigris.org;
> Patrick Mayweg; rooneg@gmail.com
> Subject: Re: r17214, JavaHL thread local storeage and reentrant calls
>
> rooneg@gmail.com wrote on 11/14/2005 03:23:54 PM:
>
> > On 11/14/05, Andrew Vaughan <aj...@netspace.net.au> wrote:
> >
> > > (I haven't studied the code, just skimmed the log, and read your
> explanation
> > > above). There should be no problems with either green or native
> threads,
> > > _iff_ the Java code is properly synchronized. (eg. either using
> > > SVNClientSynchronized.java or synchronizing on some other Object
> themselves.)
> >
> > But what if they were using different SVNClientSynchronized objects
> > entirely? It seems like there are any number of different ways you
> > could have two separate threads calling into JavaHL without any
> > locking at all between them, which would mean you're vulnerable to
> > this sort of problem.
>
> I do not believe that SVNClientSynchronized is synchronizing
> on a specific instance of the class. If it were, then it
> wouldn't be thread safe at all. It is synchrnonizing on a
> static instance of the .class which I believe will apply to
> all instances of the class and therefore all threads.
>
> We use SVNClientSynchronized in Subclipse.
>
> Mark
>
>
>
>
> ______________________________________________________________
> _______________
> Scanned for SoftLanding Systems, Inc. and SoftLanding Europe
> Plc by IBM Email Security Management Services powered by MessageLabs.
> ______________________________________________________________
> _______________
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Mark Phippard <ma...@softlanding.com>.
rooneg@gmail.com wrote on 11/14/2005 03:23:54 PM:
> On 11/14/05, Andrew Vaughan <aj...@netspace.net.au> wrote:
>
> > (I haven't studied the code, just skimmed the log, and read your
explanation
> > above). There should be no problems with either green or native
threads,
> > _iff_ the Java code is properly synchronized. (eg. either using
> > SVNClientSynchronized.java or synchronizing on some other Object
themselves.)
>
> But what if they were using different SVNClientSynchronized objects
> entirely? It seems like there are any number of different ways you
> could have two separate threads calling into JavaHL without any
> locking at all between them, which would mean you're vulnerable to
> this sort of problem.
I do not believe that SVNClientSynchronized is synchronizing on a specific
instance of the class. If it were, then it wouldn't be thread safe at
all. It is synchrnonizing on a static instance of the .class which I
believe will apply to all instances of the class and therefore all
threads.
We use SVNClientSynchronized in Subclipse.
Mark
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Andrew Vaughan <aj...@netspace.net.au> wrote:
> (I haven't studied the code, just skimmed the log, and read your explanation
> above). There should be no problems with either green or native threads,
> _iff_ the Java code is properly synchronized. (eg. either using
> SVNClientSynchronized.java or synchronizing on some other Object themselves.)
But what if they were using different SVNClientSynchronized objects
entirely? It seems like there are any number of different ways you
could have two separate threads calling into JavaHL without any
locking at all between them, which would mean you're vulnerable to
this sort of problem.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Andrew Vaughan <aj...@netspace.net.au>.
On Tue, 15 Nov 2005 05:00, Garrett Rooney wrote:
> JavaHL uses thread local storeage to keep track of a variety of
> things, for example it stores Java level exceptions there, so it's
> possible to say "an exception was thrown" in some C code, and then
> later it gets reported to the JVM. This is all stored in this
> JNIThreadData object, which is created when we enter a JavaHL function
> and destroyed when we leave it. Now the problem comes from when we
> call a JavaHL function from another JavaHL function, there's only one
> JNIThreadData object per thread, so they stomp all over each other.
> The solution was to make the JNIThreadData object really a linked
> list, so when we start the second call into a JavaHL function we just
> push another JNIThreadData onto the stack, use it for this call, then
> pop it off at the end leaving the old one there for the existing call
> to pick up and use.
>
> Now the problem becomes one of the mappings between the C level APR
> calls JavaHL makes to manipulate the thread local storeage and the
> Java level threads that are actually making the calls into JavaHL. If
> a C level thread and a Java level thread map to the same "thing"
> underneath, we can reasonably expect the C level thread local storeage
> to work correctly, but if the JVM is doing something fancy, mapping
> multiple Java level threads to a single OS level thread (a N-M
> threading scheme, green threads, whatever you want to call it), then
> it seems like this house of cards is likely to come crashing down.
>
(I haven't studied the code, just skimmed the log, and read your explanation
above). There should be no problems with either green or native threads,
_iff_ the Java code is properly synchronized. (eg. either using
SVNClientSynchronized.java or synchronizing on some other Object themselves.)
Note that deadlock is still possible.
> So is this something we need to care about? I really don't know
> enough about Java/JNI to say one way or the other, but it seems weird
> enough to be concerned about.
>
Caring about possible threading issues is definitely worth doing.
There are green thread Java implementations available (eg Kaffe).
Thanks for caring.
Andrew V.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 03:40:33PM -0800, Garrett Rooney wrote:
> On 11/14/05, Malcolm Rowe <ma...@farside.org.uk> wrote:
>
> > In which case, I'll wait until later tomorrow to see if anyone has any
> > objections, then merge it into the 1.3 branch. As I understand it,
> > a change exclusively under bindings (like this one) only requires a
> > single +1 to merge.
>
> Actually, you need one more vote to merge, someone with commit access
> to that part of the tree needs to vote at least +0, which would leave
> you with the required one +1 and one +0 vote.
Oops, yes, you're right. I didn't read the requirements properly.
> If I have a chance I'll
> try to go over all this stuff once more later today, so I can at least
> give you that ;-)
>
Thanks!
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Patrick Mayweg <ma...@qint.de>.
Thanks Malcom,
Patrick
Malcolm Rowe wrote:
>On Mon, Nov 14, 2005 at 03:40:33PM -0800, Garrett Rooney wrote:
>
>
>>Actually, you need one more vote to merge, someone with commit access
>>to that part of the tree needs to vote at least +0, which would leave
>>you with the required one +1 and one +0 vote. If I have a chance I'll
>>try to go over all this stuff once more later today, so I can at least
>>give you that ;-)
>>
>>
>
>Committed to the 1.3.x branch in r17350. Thanks for the help!
>
>Regards,
>Malcolm
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 03:40:33PM -0800, Garrett Rooney wrote:
> Actually, you need one more vote to merge, someone with commit access
> to that part of the tree needs to vote at least +0, which would leave
> you with the required one +1 and one +0 vote. If I have a chance I'll
> try to go over all this stuff once more later today, so I can at least
> give you that ;-)
Committed to the 1.3.x branch in r17350. Thanks for the help!
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Nov 15, 2005 at 07:40:09AM +0100, Patrick Mayweg wrote:
> SVNClientSynchronized synchronizes on its class object, which can only
> exists once per class loader. It should be changed to the SVNClient
> class object, because that can only exists once per JVM. If you keep
> both classes in one Jar, that is not a real problem. But that is nothing
> new. On the other hand, using synchronization prevents only other
> threads to access something. It does not prevent reentracy.
>
Ok, that change makes sense, though I still don't understand why we need
such strict synchronisation. At the moment, SVNClientSynchronized is
acting as a barrier that allows only one call to be active per JVM,
rather than one per SVNClient object (as would be the case if the
SVNClient object wasn't thread-safe itself).
> I wrote SVNClientSynchronized in the first place, because I did see any
> statement about the subversion core libraries reentrant. So if any
> problems arise, it should be easy to switch a thread safe version.
The requirements for re-entrancy are pretty much a subset of the
requirements for thread safety, and with callbacks, re-entrancy problems
would show up even on a single thread. The Subversion core libraries
can be used in multi-threaded environments in both server configurations,
I believe.
> I will change JavaHL, so that JNIThreadData will no longer be stored in
> thread local storage but passed as parameter in almost all methods and
> put it into the batons. So that every call into the native library will
> keep its JNIEnv, no matter how the JVM schedules the java/native thread.
> But this is major change. I will not have time to do that for 1.3 and I
> do not think it should go 1.3 anyway.
>
I'm not sure that would be an improvement: you'd just be changing the
current explicit thread-local stack for one managed through the thread's
(thread-local) function-call stack, and the latter would have a lot
more duplication.
Then again, passing the JNIEnv to each function is easier to understand,
and it does remove a dependency on the APR TLS APIs. And I guess that
it also reduces the pressure on the (limited number of) TLS slots (and
the JNI spec explicitly mentions that JVMs can use thread-local storage
for the JNIEnv data, so we should expect that some will be in use).
But regardless of whether it's worthwhile in itself, you're right that
a change of that magnitude (even if it _is_ mostly just repetitive)
shouldn't go into 1.3, unless we've got an indication that it actually
fixes something (and at the moment, I can't see that it will have any
real effect at all).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Patrick Mayweg <ma...@qint.de> wrote:
> Hi Everyone,
> unfortunately this discussion started in a time, where I normaly do not
> sit in front of a computer. That why I am joining so late.
No worries, everyone gets busy from time to time.
> I did not vote for my change, because I have difficulty to commit since
> I am working at a customers site. I also had to work thru the last
> weekend and simply forgot to it then. I think the change is an
> improvment for 1.3 and should go into 1.3.
>
> SVNClientSynchronized synchronizes on its class object, which can only
> exists once per class loader. It should be changed to the SVNClient
> class object, because that can only exists once per JVM. If you keep
> both classes in one Jar, that is not a real problem. But that is nothing
> new. On the other hand, using synchronization prevents only other
> threads to access something. It does not prevent reentracy.
>
> I wrote SVNClientSynchronized in the first place, because I did see any
> statement about the subversion core libraries reentrant. So if any
> problems arise, it should be easy to switch a thread safe version.
For what it's worth, the Subversion libraries internally are supposed
to be thread safe. Assuming you are using separate context objects
(i.e. if you're calling a client library function in two separate
threads you need two separate svn_client_ctx_t objects, similar rules
apply for other state that gets passed into subversion functions) you
should be fine.
> I will change JavaHL, so that JNIThreadData will no longer be stored in
> thread local storage but passed as parameter in almost all methods and
> put it into the batons. So that every call into the native library will
> keep its JNIEnv, no matter how the JVM schedules the java/native thread.
> But this is major change. I will not have time to do that for 1.3 and I
> do not think it should go 1.3 anyway.
That seems best, but in the meantime the current change should be fine for 1.3.
Thanks,
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Patrick Mayweg <ma...@qint.de>.
Hi Everyone,
unfortunately this discussion started in a time, where I normaly do not
sit in front of a computer. That why I am joining so late.
I did not vote for my change, because I have difficulty to commit since
I am working at a customers site. I also had to work thru the last
weekend and simply forgot to it then. I think the change is an
improvment for 1.3 and should go into 1.3.
SVNClientSynchronized synchronizes on its class object, which can only
exists once per class loader. It should be changed to the SVNClient
class object, because that can only exists once per JVM. If you keep
both classes in one Jar, that is not a real problem. But that is nothing
new. On the other hand, using synchronization prevents only other
threads to access something. It does not prevent reentracy.
I wrote SVNClientSynchronized in the first place, because I did see any
statement about the subversion core libraries reentrant. So if any
problems arise, it should be easy to switch a thread safe version.
I will change JavaHL, so that JNIThreadData will no longer be stored in
thread local storage but passed as parameter in almost all methods and
put it into the batons. So that every call into the native library will
keep its JNIEnv, no matter how the JVM schedules the java/native thread.
But this is major change. I will not have time to do that for 1.3 and I
do not think it should go 1.3 anyway.
Regards,
Patrick
Garrett Rooney wrote:
>On 11/14/05, Malcolm Rowe <ma...@farside.org.uk> wrote:
>
>
>
>>Thanks. (HACKING doesn't actually define what a -0 means, or whether
>>it's a veto.)
>>
>>
>
>-0 is more of a "I'm not sure this is a good idea" kind of thing.
>
>
>
>>In which case, I'll wait until later tomorrow to see if anyone has any
>>objections, then merge it into the 1.3 branch. As I understand it,
>>a change exclusively under bindings (like this one) only requires a
>>single +1 to merge.
>>
>>
>
>Actually, you need one more vote to merge, someone with commit access
>to that part of the tree needs to vote at least +0, which would leave
>you with the required one +1 and one +0 vote. If I have a chance I'll
>try to go over all this stuff once more later today, so I can at least
>give you that ;-)
>
>-garrett
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Malcolm Rowe <ma...@farside.org.uk> wrote:
> Thanks. (HACKING doesn't actually define what a -0 means, or whether
> it's a veto.)
-0 is more of a "I'm not sure this is a good idea" kind of thing.
> In which case, I'll wait until later tomorrow to see if anyone has any
> objections, then merge it into the 1.3 branch. As I understand it,
> a change exclusively under bindings (like this one) only requires a
> single +1 to merge.
Actually, you need one more vote to merge, someone with commit access
to that part of the tree needs to vote at least +0, which would leave
you with the required one +1 and one +0 vote. If I have a chance I'll
try to go over all this stuff once more later today, so I can at least
give you that ;-)
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 03:23:47PM -0800, Garrett Rooney wrote:
> > Out of interest, does dlr's -0 act as a veto, or is the change approved
> > regardless? (I'd still prefer we didn't merge it until everyone's happy,
> > of course, but I just wonder what it means in practice).
>
> A -0 does not block the merge.
>
Thanks. (HACKING doesn't actually define what a -0 means, or whether
it's a veto.)
In which case, I'll wait until later tomorrow to see if anyone has any
objections, then merge it into the 1.3 branch. As I understand it,
a change exclusively under bindings (like this one) only requires a
single +1 to merge.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Mon, Nov 14, 2005 at 10:41:10PM +0000, Malcolm Rowe wrote:
> > [...]
>
> In fact, after going through all this again, I've just reviewed the change
> itself, and I'm happy that I understand it thoroughly enough that I've
> just voted +1 on it in STATUS.
Thanks!
> Out of interest, does dlr's -0 act as a veto, or is the change approved
> regardless? (I'd still prefer we didn't merge it until everyone's happy,
> of course, but I just wonder what it means in practice).
A -0 does not block the merge.
> (Regarding the issue mentioned in the STATUS log message: "concurrent
> calls into the JNI library not completing in LIFO order". This won't
> be a problem either, since concurrent calls into JNI must originate
> from different Java threads, and to be concurrent, must be running on
> different OS threads, so there'll be a different linked-list for each
> OS thread).
Yes, that makes sense to me.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 10:41:10PM +0000, Malcolm Rowe wrote:
> [...]
In fact, after going through all this again, I've just reviewed the change
itself, and I'm happy that I understand it thoroughly enough that I've
just voted +1 on it in STATUS.
Out of interest, does dlr's -0 act as a veto, or is the change approved
regardless? (I'd still prefer we didn't merge it until everyone's happy,
of course, but I just wonder what it means in practice).
(Regarding the issue mentioned in the STATUS log message: "concurrent
calls into the JNI library not completing in LIFO order". This won't
be a problem either, since concurrent calls into JNI must originate
from different Java threads, and to be concurrent, must be running on
different OS threads, so there'll be a different linked-list for each
OS thread).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 01:49:39PM -0800, Garrett Rooney wrote:
> > I admit I'm assuming that the JVM will block execution of the Java thread
> > while the JNI call is executing, but I think that's fair. I can't think
> > of anything else, so I can only conclude that I'm missing something,
> > or it's not a problem.
>
> See, that's what I'm asking. I don't know enough about JNI to make
> those sort of statements, so I'm asking for someone who does know it
> to step up and say "don't worry, that's not a problem".
>
I think that it's fair to assume the JVM will block the Java thread
while the JNI call is executing (or at least, act as-if it does).
I've just skimmed the JNI spec, and it doesn't suggest anything other
than the obvious synchronous execution model (and what would the JVM do
with the Java thread while the JNI call is executing, in any case?)
[Java doesn't have the equivalent of COMs asynchronous method calls,
as far as I'm aware, and even if it does, they're sure to be implemented
by using additional Java (worker) threads. The JNI spec doesn't mention
them.]
I think that as long as we don't use multiple threads within the JNI call
itself (and we don't, as far as I can see: neither our JNI implementation
nor our libraries create new threads), we should be okay. There's a note
about the ability for other threads to be able to 'post' an exception
to a thread executing within a JNI call ('asynchronous exceptions'),
but I don't think that's relevant.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Malcolm Rowe <ma...@farside.org.uk> wrote:
> I admit I'm assuming that the JVM will block execution of the Java thread
> while the JNI call is executing, but I think that's fair. I can't think
> of anything else, so I can only conclude that I'm missing something,
> or it's not a problem.
See, that's what I'm asking. I don't know enough about JNI to make
those sort of statements, so I'm asking for someone who does know it
to step up and say "don't worry, that's not a problem".
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Nov 14, 2005 at 10:00:45AM -0800, Garrett Rooney wrote:
> Now the problem becomes one of the mappings between the C level APR
> calls JavaHL makes to manipulate the thread local storeage and the
> Java level threads that are actually making the calls into JavaHL. If
> a C level thread and a Java level thread map to the same "thing"
> underneath, we can reasonably expect the C level thread local storeage
> to work correctly, but if the JVM is doing something fancy, mapping
> multiple Java level threads to a single OS level thread (a N-M
> threading scheme, green threads, whatever you want to call it), then
> it seems like this house of cards is likely to come crashing down.
Sorry - am I missing something here? Perhaps someone could explain why
this could cause a problem, because I don't see it yet.
JavaHL is using TLS in its implementation to store a context, effectively,
for a given call (I assume that this is an implementation-defined part
of JavaHL, and not part of the JNI interface?). The problem seems
to be that we don't know which OS thread will be used by the JVM for
a Java-JNI (for example, it might be 1:1, N:M [thread pools], or N:1
[old-style Mac OS 9 JVM, or 'green threads', whatever they are]).
I assume that the JVM can't preempt the call while it's wholly within
the JNI execution stack (since I don't see how it could), so this means
that whatever OS thread the JVM chooses to execute the JNI call, that OS
thread will be used exclusively by that Java thread for the duration of
the call. Even if we call back into the JVM and out again into another
native method (where we might get called on a different OS thread),
we won't need to pass the context object back from the second native
call to the first, will we?
I admit I'm assuming that the JVM will block execution of the Java thread
while the JNI call is executing, but I think that's fair. I can't think
of anything else, so I can only conclude that I'm missing something,
or it's not a problem.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
> I guess from my point of view, my concern is that I would like to see this
> patch applied to 1.3.x so that it can be picked up in the next release
> candidate and we can have a better 1.3.x release of Subclipse.
That's fine, but considering that I can hardly even understand what
the patch does, I'm certainly not about to vote for it myself,
especially if the author of the patch hasn't even stepped up to say
"yes, I think this should be in 1.3.x" himself.
> I am not a C programmer but it doesn't seem to me that Patrick's change
> have introduced any new problems. The problems that you are concerned
> about would exist in the existing code as well. (wouldn't they?) So
> while I think it is good that you have potentially identified a problem
> scenario and are looking for an approach that will fix it, I would still
> like to see Patrick's patch applied to the branch.
Yes, it does seem like a step in the right direction, it's at the very
least better than it was, but I really don't understand this code well
enough to approve anything more than trivial changes to it, and this
is most certainly non-trivial stuff. Someone with some background in
the JavaHL code really needs to step up and say "this is good, for
this reason, so we should backport it" before I'll feel comfortable
about it.
> That being said, it was dlr that vetoed the patch so it could be that I am
> making an incorrect assumption in linking it to your email. I am still
> waiting to hear what dlr's reason was for the veto.
I believe DLR has changed his vote to a -0, as opposed to the previous -1.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Mark Phippard <ma...@softlanding.com>.
rooneg@gmail.com wrote on 11/14/2005 01:25:24 PM:
> On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
>
> > I applied Patrick's patch to the 1.3.x branch on Windows and built it.
It
> > passed all of the JUnit tests. I then put the code into Subclipse and
> > added reentry code. Namely, I removed all internal caching of calls
to
> > getAdminDirectory() and isAdminDirectory(). I also put the cancel
> > operation code back in. With Patrick's patch everything appears to
work
> > properly now. Having support for cancel has been one of the top
requests
> > for Subclipse so it would be good to be able to add this feature.
>
> Oh, I have no question that this is an improvement, I just want to
> verify that there aren't other demons lurking out in the darkness
> waiting to eat us alive at some point in the future.
I guess from my point of view, my concern is that I would like to see this
patch applied to 1.3.x so that it can be picked up in the next release
candidate and we can have a better 1.3.x release of Subclipse.
I am not a C programmer but it doesn't seem to me that Patrick's change
have introduced any new problems. The problems that you are concerned
about would exist in the existing code as well. (wouldn't they?) So
while I think it is good that you have potentially identified a problem
scenario and are looking for an approach that will fix it, I would still
like to see Patrick's patch applied to the branch.
That being said, it was dlr that vetoed the patch so it could be that I am
making an incorrect assumption in linking it to your email. I am still
waiting to hear what dlr's reason was for the veto.
Mark
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/14/05, Mark Phippard <ma...@softlanding.com> wrote:
> I applied Patrick's patch to the 1.3.x branch on Windows and built it. It
> passed all of the JUnit tests. I then put the code into Subclipse and
> added reentry code. Namely, I removed all internal caching of calls to
> getAdminDirectory() and isAdminDirectory(). I also put the cancel
> operation code back in. With Patrick's patch everything appears to work
> properly now. Having support for cancel has been one of the top requests
> for Subclipse so it would be good to be able to add this feature.
Oh, I have no question that this is an improvement, I just want to
verify that there aren't other demons lurking out in the darkness
waiting to eat us alive at some point in the future.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r17214, JavaHL thread local storeage and reentrant calls
Posted by Mark Phippard <ma...@softlanding.com>.
rooneg@gmail.com wrote on 11/14/2005 01:00:45 PM:
> So is this something we need to care about? I really don't know
> enough about Java/JNI to say one way or the other, but it seems weird
> enough to be concerned about.
I am also somewhat involved in this. Let me just share the bits I know
....
In debugging the original problem for Patrick I determined that in the
case of Subclipse (which was triggering the problem) this wasn't a thread
issue, but a reentry issue. As an example, Subclipse would run checkout,
which run the native checkout code, which would send notifications back to
Java which would call back into the code to run getAdminDirectory(). This
would crash. Talking to Patrick I realized that this was also likely the
problem with another crash scenario. When we tried to add support in
Subclipse for cancelling an operation it would crash. I just assumed
JavaHL "worked" and the bug was in Subclipse so I set it aside and did not
report it.
I applied Patrick's patch to the 1.3.x branch on Windows and built it. It
passed all of the JUnit tests. I then put the code into Subclipse and
added reentry code. Namely, I removed all internal caching of calls to
getAdminDirectory() and isAdminDirectory(). I also put the cancel
operation code back in. With Patrick's patch everything appears to work
properly now. Having support for cancel has been one of the top requests
for Subclipse so it would be good to be able to add this feature.
Unfortunately, I cannot comment on your concerns. I can only give you
these observations.
Thanks
Mark
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org