Re: PATCH: System properties and init_properties

[Bryce McKinlay <bryce at albatross dot co dot nz>]

Tom Tromey wrote:

> I looked here:
>
>     http://java.sun.com/products/jdk/1.2/docs/api/java/lang/System.html
>
> According to this, setProperties(null) should forget the current set
> of system properties.  My reading is that the next getProperties()
> should cause the system properties to be recomputed.
>
> What do you think?
>
> If my reading holds, then we should just get rid of prop_init and
> always reinitialize the system properties when properties==null.

ok, I think the semantics were correct, but I agree we don't need
prop_init, and I noticed that the existing code had thread safety issues.
Here's what I'm checking in.

regards

  [ bryce ]



2000-11-24  Bryce McKinlay  <bryce@albatross.co.nz>

	* java/lang/System.java (setProperties): Only call init_properties()
	if properties is null.	
	(getProperties): Ditto.
	(getProperty): Ditto.
	(setProperty): Call init_properties if properties are null.
	(prop_init): Remove field.
	* java/lang/natSystem.cc (init_properties): Synchronize the entire
	method. Check for null properties after synchronizing instead of 
	prop_init flag. Set the properties field last for thread safety.

Index: System.java
===================================================================
RCS file: /cvs/java/libgcj/libjava/java/lang/System.java,v
retrieving revision 1.4
diff -u -r1.4 System.java
--- System.java	2000/08/21 06:05:19	1.4
+++ System.java	2000/11/26 01:28:38
@@ -63,7 +63,8 @@
   {
     if (secman != null)
       secman.checkPropertiesAccess();
-    init_properties ();
+    if (properties == null)
+      init_properties ();
     return properties;
   }
 
@@ -71,7 +72,8 @@
   {
     if (secman != null)
       secman.checkPropertyAccess(property);
-    init_properties ();
+    if (properties == null)
+      init_properties ();
     return properties.getProperty(property);
   }
 
@@ -79,7 +81,8 @@
   {
     if (secman != null)
       secman.checkPropertyAccess(property, defval);
-    init_properties ();
+    if (properties == null)
+      init_properties ();
     return properties.getProperty(property, defval);
   }
 
@@ -128,15 +131,18 @@
   {
     if (secman != null)
       secman.checkPropertiesAccess();
-    // We might not have initialized yet.
-    prop_init = true;
-    properties = props;
+    synchronized (System.class)
+    {
+      properties = props;
+    }
   }
 
   public static String setProperty (String key, String value)
   {
     if (secman != null)
       secman.checkPermission (new PropertyPermission (key, "write"));
+    if (properties == null)
+      init_properties ();
     return (String) properties.setProperty (key, value);
   }
 
@@ -165,7 +171,4 @@
   // Private data.
   private static SecurityManager secman = null;
   private static Properties properties = null;
-  // This boolean is only required for 1.1 and earlier.  After 1.1, a
-  // null properties should always be re-initialized.
-  private static boolean prop_init = false;
 }
Index: natSystem.cc
===================================================================
RCS file: /cvs/java/libgcj/libjava/java/lang/natSystem.cc,v
retrieving revision 1.29
diff -u -r1.29 natSystem.cc
--- natSystem.cc	2000/10/06 01:49:31	1.29
+++ natSystem.cc	2000/11/26 01:28:38
@@ -217,18 +217,16 @@
 void
 java::lang::System::init_properties (void)
 {
-  {
-    // We only need to synchronize around this gatekeeper.
-    JvSynchronize sync (&java::lang::System::class$);
-    if (prop_init)
-      return;
-    prop_init = true;
-  }
+  JvSynchronize sync (&java::lang::System::class$);
+  
+  if (properties != NULL)
+    return;
 
-  properties = new java::util::Properties ();
+  java::util::Properties* newprops = new java::util::Properties ();
+  
   // A convenience define.
 #define SET(Prop,Val) \
-	properties->put(JvNewStringLatin1 (Prop), JvNewStringLatin1 (Val))
+	newprops->put(JvNewStringLatin1 (Prop), JvNewStringLatin1 (Val))
 
   // A mixture of the Java Product Versioning Specification
   // (introduced in 1.2), and earlier versioning properties.
@@ -351,7 +349,7 @@
 	;
       jstring name = JvNewStringLatin1 (p, s - p);
       jstring val = JvNewStringLatin1 (*s == '=' ? s + 1 : s);
-      properties->put (name, val);
+      newprops->put (name, val);
     }
 
   // Set the system properties from the user's environment.
@@ -368,13 +366,13 @@
     }
 
   if (_Jv_Jar_Class_Path)
-    properties->put(JvNewStringLatin1 ("java.class.path"),
-		    JvNewStringLatin1 (_Jv_Jar_Class_Path));
+    newprops->put(JvNewStringLatin1 ("java.class.path"),
+		  JvNewStringLatin1 (_Jv_Jar_Class_Path));
   else
     {
       // FIXME: find libgcj.zip and append its path?
       char *classpath = ::getenv("CLASSPATH");
-      jstring cp = properties->getProperty (JvNewStringLatin1("java.class.path"));
+      jstring cp = newprops->getProperty (JvNewStringLatin1("java.class.path"));
       java::lang::StringBuffer *sb = new java::lang::StringBuffer ();
       
       if (classpath)
@@ -391,7 +389,11 @@
       else
 	sb->append ((jchar) '.');
       
-      properties->put(JvNewStringLatin1 ("java.class.path"),
+      newprops->put(JvNewStringLatin1 ("java.class.path"),
 		      sb->toString ());
     }
+  // Finally, set the field. This ensures that concurrent getProperty() 
+  // calls will return initialized values without requiring them to be 
+  // synchronized in the common case.
+  properties = newprops;
 }