Ok, I just could not take it anymore so I rewrote JavaGetEnv. The old implementation used a method for JDK 1.1 and another completely different method for JDK 1.2. The methods were almost the same but still different. Needless to say, this was not a good situation at all. The new impl uses #ifdefs for things that need to be changed for JDK1_2. While I was in there, I noticed that there is a synchronization case that is not covered by the approach advocated in: http://www-cs-students.stanford.edu/~jwu/blendchanges.txt Loading Tcl Blend from Tcl is fine, but may be a race condition when two Java threads create a Tcl interp at the same time. >From section 3.2 in blendchanges.txt: load "tclblend", "tcl" .dll or .so (a) call "new Interp()" in Java invoke Java_tcl_lang_Interp_create() C function (b) calls JavaSetupJava() invoke Java_tcl_lang_Interp_init() C function (c) We do need a mutex around the init of tsdPtr->currentEnv and tsdPtr->initialized_currentEnv. These are set the first time JavaGetEnv() is called, but JavaGetEnv() is not called when Tcl Blend is loaded from Java. I think we should put a mutex inside the JavaGetEnv() method. This will protect us from the race condition and it should not slow down later calls. I also used the same lock inside JavaSetupJava. (I know, I know, I used a goto, I feel dirty). Index: src/native/javaCmd.c =================================================================== RCS file: /home/cvs/external/tcljava/src/native/javaCmd.c,v retrieving revision 1.9.2.3 diff -u -r1.9.2.3 javaCmd.c --- javaCmd.c 2000/08/07 08:50:16 1.9.2.3 +++ javaCmd.c 2000/08/07 09:27:07 @@ -10,7 +10,7 @@ * of this file, and for a DISCLAIMER OF ALL WARRANTIES. * * - * RCS: @(#) $Id: javaCmd.c,v 1.9.2.3 2000/08/07 08:50:16 mo Exp $ + * RCS: @(#) $Id: javaCmd.c,v 1.9.2.3 2000/08/07 00:50:09 mo Exp $ */ /* @@ -81,6 +81,19 @@ /* Define this here so that we do not need to include tclInt.h */ #define TCL_TSD_INIT(keyPtr) (ThreadSpecificData *)Tcl_GetThreadData((keyPtr), sizeof(ThreadSpecificData)) +/* + * Declare a global mutex to protect the creation and initialization of the + * JVM from mutiple threads. This mutex is used in conjunction with the + * 'initialized_javaVM' flag. This mutex is used in javaCmd.c as well as + * javaInterp.c. + * + * FIXME: don't want to use the flag TCL_THREADS explicitly. This may be + * better if in the future the same TclBlend binary can be made to work with + * both threaded and non-threaded Tcl libraries. For now, we will use accessor + * functions lockJVMInitMutex() and unlockJVMInitMutex(). + */ +TCL_DECLARE_MUTEX(javaVM_init_mutex) + /* * The following variable contains the pointer to the current Java VM, @@ -385,12 +398,29 @@ ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey); + /* + * Don't acquire the mutex here! We need JavaGetEnv() to be fast in + * the case where tsdPtr->currentEnv has already been initialized. + */ + if (tsdPtr->initialized_currentEnv) { return tsdPtr->currentEnv; } - /* FIXME : we need to put a mutex around this create JVM/attach step */ + Tcl_MutexLock(&javaVM_init_mutex); + /* + * Check to see if the JNIEnv was initialized again after + * entering the critical section in case two threads were + * racing to initialize the tsdPtr->currentEnv + */ + + if (tsdPtr->initialized_currentEnv) { + Tcl_MutexUnlock(&javaVM_init_mutex); + return tsdPtr->currentEnv; + } + + #ifdef TCLBLEND_DEBUG fprintf(stderr, "TCLBLEND_DEBUG: JavaGetEnv for %s JVM\n", #ifdef JDK1_2 @@ -415,7 +445,7 @@ #endif /* TCLBLEND_DEBUG */ Tcl_AppendResult(interp, "JNI_GetCreatedJavaVMs failed", NULL); - return NULL; + goto error; } if (nVMs == 0) { @@ -523,7 +553,7 @@ "Tcl Blend only: If the value is 'help', then JVM initialization stop\n", "and this message is returned.", NULL); - return NULL; + goto error; } options[vm_args.nOptions].extraInfo = (void *)NULL; vm_args.nOptions++; @@ -568,7 +598,7 @@ "than the one Tcl Blend was compiled with?\n", NULL); appendClasspathMessage(interp); - return NULL; + goto error; } } else { @@ -589,12 +619,17 @@ #endif /* TCLBLEND_DEBUG */ Tcl_AppendResult(interp, "AttachCurrentThread failed", NULL); - return NULL; + goto error; } } tsdPtr->initialized_currentEnv = 1; + Tcl_MutexUnlock(&javaVM_init_mutex); return tsdPtr->currentEnv; + + error: + Tcl_MutexUnlock(&javaVM_init_mutex); + return NULL; } /* @@ -739,14 +774,18 @@ jfieldID field; int i; + /* + * Acquire the init lock, we do not care if it is slow to call + * JavaSetupJava, it is only called when an Interp is created + */ + Tcl_MutexLock(&javaVM_init_mutex); #ifdef TCLBLEND_DEBUG fprintf(stderr, "TCLBLEND_DEBUG: called JavaSetupJava\n"); #endif /* TCLBLEND_DEBUG */ - - /* FIXME : we need to put a mutex around this test/set */ if (initialized_javaVM) { + Tcl_MutexUnlock(&javaVM_init_mutex); return TCL_OK; } @@ -858,10 +897,12 @@ JavaObjInit(); initialized_javaVM = 1; + Tcl_MutexUnlock(&javaVM_init_mutex); return TCL_OK; error: (*env)->ExceptionClear(env); + Tcl_MutexUnlock(&javaVM_init_mutex); return TCL_ERROR; } Any comments on this approach? I like it a bit better than grabbing the lock before calling JavaGetEnv or JavaSetupJava, the code seems a bit more clean with the synchronization inside the methods. Mo DeJong Red Hat Inc ---------------------------------------------------------------- The TclJava mailing list is sponsored by Scriptics Corporation. To subscribe: send mail to [EMAIL PROTECTED] with the word SUBSCRIBE as the subject. To unsubscribe: send mail to [EMAIL PROTECTED] with the word UNSUBSCRIBE as the subject. To send to the list, send email to '[EMAIL PROTECTED]'. An archive is available at http://www.mail-archive.com/tcljava@scriptics.com
