On Nov 26, 2007 9:37 AM, Mark Phippard <markphip@gmail.com> wrote:
> I have been getting some crashes lately in Subclipse (using 1.5
> trunk). Kind of sporadic, not obvious recipe. I finally bothered to
> take the time to look at the crash output (this is on OSX). I saw
> this:
>
> Thread 22 Crashed:
> 0 libjvm.dylib 0x9bc16fb8 JVM_MonitorWait + 11444
> 1 libjvm.dylib 0x9bcd98bb JVM_IsInterrupted + 1797
> 2 libsvnjavahl-1.0.dylib 0x50cf8f6c
> JNIEnv_::CallVoidMethod(_jobject*, _jmethodID*, ...) + 52 (jni.h:1034)
> 3 libsvnjavahl-1.0.dylib 0x50cde64d
> ProgressListener::onProgress(long long, long long, apr_pool_t*) + 495
> (ProgressListener.cpp:128)
> 4 libsvnjavahl-1.0.dylib 0x50cde6c6
> ProgressListener::progress(long long, long long, void*, apr_pool_t*) +
> 82 (ProgressListener.cpp:77)
> 5 libsvn_ra_neon-1.0.dylib 0x50c946f0 ra_neon_neonprogress + 99
> 6 libneon.26.dylib 0x515e2f0f ne_read_response_block + 145
> 7 libneon.26.dylib 0x515e33b5 ne_discard_response + 34
> 8 libneon.26.dylib 0x515e45aa ne_request_dispatch + 46
> 9 libsvn_ra_neon-1.0.dylib 0x50c98c18 svn_ra_neon__request_dispatch + 238
> 10 libsvn_ra_neon-1.0.dylib 0x50c98598 parsed_request + 577
> 11 libsvn_ra_neon-1.0.dylib 0x50c987ba svn_ra_neon__parsed_request + 114
> 12 libsvn_ra_neon-1.0.dylib 0x50c8a71c reporter_finish_report + 539
> 13 libsvn_client-1.0.dylib 0x512e3190 reporter_finish_report + 433
> 14 libsvn_wc-1.0.dylib 0x50ffeff0 svn_wc_crawl_revisions3 + 1613
> 15 libsvn_client-1.0.dylib 0x512e380a svn_client_status3 + 1605
> 16 libsvnjavahl-1.0.dylib 0x50ce74c5 SVNClient::status(char
> const*, svn_depth_t, bool, bool, bool, bool, StatusCallback*) + 335
> (SVNClient.cpp:172)
> 17 libsvnjavahl-1.0.dylib 0x50cef3b1
> Java_org_tigris_subversion_javahl_SVNClient_status + 323
> (org_tigris_subversion_javahl_SVNClient.cpp:179)
>
> I think I know what the problem is. We recently started using the new
> progress notifications. I think what happens is I take an option to
> do an Update. We set the progress listener so that we can provide
> feedback. At some point during this process, Eclipse kicks off a
> status refresh in another thread (also normal). There is only one
> JavaHL object. When the status operation starts, the progress
> listener is still set because update is running. When update ends, it
> is nulling out the progress listener.
>
> SVNClient.setProgressListener(null);
>
> Status goes to send a progress notification, and something is now null.
>
> I have changed Subclipse so that we no longer null out the listener,
> and instead just swallow the notifications that we do not want inside
> our object. I wonder if there is some kind of safety checks that can
> be added in the JavaHL C++ code though so that it does not crash. I
> do not think what I am doing is that abnormal so someone else might
> run into this.
Dan and I discussed this on IRC. Here is transcript for archives:
<dlr> markphip: Around? I wanted to understand more about that
threading issue you ran into w/ usage of the JavaHL API.
<dlr> The Subclipse usage was nuking svn_client_ctx_t->progress_func
while it was being used by multiple threads.
<markphip> that was my guess
<dlr> AIUI, the C API assumes one svn_client_ctx_t per thread.
<dlr> That would mean one SVNClient per thread.
<markphip> there is only one instance of SVNClient and one setProgress
and we set it to null
<markphip> hmm, we talked about this once before and that is what had
me change it to be like this
<markphip> we used to create a new SVNClient everytime we wanted to do something
<dlr> I recall
<markphip> we do not directly deal with threads in Eclipse, but I
guess maybe we could determine the current thread and have a hashmap
or something
<dlr> For operations which don't change the state of the
svn_client_ctx_t, that should work fine.
<markphip> I do not think we would know when the threads end though
<dlr> But, if you've got multiple threads using the same
svn_client_ctx_t, you can't change it and expect things to work.
<markphip> what is svn_client_ctx_t in layman's terms?
<dlr> In Java terms, it's an Interface.
<dlr> ...implemented by clients, to provide Subversion behaviors
custom to those clients.
<markphip> but in SVN terms, what things are part of it? The
configuration area type stuff?
<dlr> markphip: http://svn.collab.net/svn-doxygen/structsvn__client__ctx__t.html
<markphip> the change I made will probably work then, I just did not
know if this could also be considered a bug ... sounds like not
<dlr> markphip: It contains a bunch of callbacks (methods on the Java
interface). So, even more than carrying configuration, it dictates
behavior.
<dlr> When you reset the progress_func field, you're overwriting one
of those behaviors, mid-operation.
<markphip> hmm, this is going to require some thought then ... sounds
like problems waiting to happen
<markphip> a lot of what is in that struct we could fairly safely not
change, but some of the things we do regularly
<dlr> If you don't write to it, like I said, you should be good.
<dlr> ok
<dlr> What else, other than progress_func and progress_baton?
<markphip> well, we add the conflict stuff during merge .. then remove it
<markphip> I think the log message callback probably only gets set
during that operation
<markphip> I suppose in both of those cases though, another thread
would not be changing it
<dlr> I don't quite understand why you need to remove the callbacks.
<dlr> You're using objects for callbacks; do these objects contain
some state that's specific to a single invocation of the callback?
<markphip> in the case of the merge one, it is related to the UI and
the callback knowing which merge it pertains to etc...
<markphip> the merge one might contain some state. The others ...
probably not so much
<dlr> then why remove them?
<markphip> the progress one has some info about what operation it was
running so it know when to clear the counters etc...
<dlr> okay, so a few callbacks have state
<markphip> I doubt we remove things like the log message callback
<markphip> but we probably replace it
<dlr> Yeah, I don't see a need there.
<dlr> That could theoretically be a problem, too.
<markphip> I'd have to look, it could always be the same object
<dlr> (see Java's double-checked locking)
<dlr> What you could do, for the guys you need to add state for, is
use a SVNClient extension that uses thread-local storage for the
callbacks.
<markphip> So if we eventually create more than one SVNClient, the
thing that started this many months ago, is that we should call
dispose() when we are done with it?
<dlr> Ideally, no matter how many you create, you should call dispose().
<dlr> Have you used Java's ThreadLocal class before?
<markphip> no
<dlr> It can be used to achieve almost "magical" behavior.
<dlr> It's really easy to mis-use, too, and results in code that's
harder for newcomers to follow.
<dlr> But for the original author, it's quite easy to use.
<dlr> Basically, it gives you a hashtable that's associated with the
"current" thread (whichever that may be).
<dlr> So when you call SVNClient.setProgressListener(), it would in
turn call ThreadLocal.set().
<dlr> ...associating the supplied ProgressListener with the current thread.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 27 02:15:21 2007