[modules-dev] Review request for 6578988

[Dave Bristor]

Thanks Stanley.  It'll take a bit longer to address comments in the other 
review, so I figured on taking care of this first.

I moved all field declarations to be at the top of the class declaration.  I 
did this after noticing them placed throughout the class, and in particular 
seeing ThreadFactory, which is now used to create the shutdownThread.

I also added isReloadSupported(), as per the spec.  Rather than make it 
abstract, I have it returning false by default; subclasses can override this.

Webrev updated:
	http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6578988

Thanks,
	Dave

Stanley M. Ho wrote:
> Hi Dave,
> 
> Looks good. One comment:
> 
> - src/share/classes/java/module/Repository.java
> 
> 244                 shutdownThread = new Thread() {
> 
> The issue is that this shutdownThread is not created until the first 
> shutdownOnExit() is called. However, this could happen in any thread 
> group. In the sandbox, thread groups easily come and go as applications 
> are loaded, and shutdown, and this shutdown hook might not be executed 
> properly if the associated thread group has been destroyed before the 
> end of the JVM session.
> 
> The proper way to fix this is to obtain the main thread group when the 
> module system is started, and make sure this shutdown thread is created 
> in the main thread group.
> 
> - Stanley
> 
> 
> Dave Bristor wrote:
>> Hi folks,
>>
>> This fix for
>>     (repo) Implement Repository.shutdownOnExit
>>
>> builds on top of my previous request (attached); it has build atop 
>> that because the changes here for shutdownOnExit() rely on the changes 
>> to shutdown() in the prior request.
>>
>> This is a *lot* shorter than my previous request.  It includes some 
>> javadoc fixes in JamUtils.
>>
>> You'll see in the change that when shutdownOnExit is invoked with 
>> false, that any previously-created shutdownThread is made available to 
>> GC.  I didn't see any reason to retain the reference if the likelihood 
>> is that it won't be used.  It might be better to keep it once 
>> created.  Space v. time!
>>
>> bugster: http://monaco.sfbay/detail.jsp?cr=6578988
>> webrev:  http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6578988
>>
>> I submitted a JPRT run that has changes for this fix plus the others.
>>
>> Thanks!
>>     Dave
>>
>> ------------------------------------------------------------------------
>>
>> Subject:
>> Review request for 6559067, 6559086, and 6605100
>> From:
>> Dave Bristor <David.Bristor at sun.com>
>> Date:
>> Fri, 05 Oct 2007 15:55:54 -0700
>> To:
>> modules-dev at openjdk.java.net
>>
>> To:
>> modules-dev at openjdk.java.net
>>
>>
>> Hi folks,
>>
>> This fixes 3 bugs:
>> (repo) implement LocalRepository.shutdown() and reload()
>>     http://monaco.sfbay/detail.jsp?cr=6559067
>>
>> (repo) URLRepository.shutdown() and reload()
>>     http://monaco.sfbay/detail.jsp?cr=6559086
>>
>> (repo) Allow factories in repository configuration
>>     http://monaco.sfbay/detail.jsp?cr=6605100
>>
>> The webrev is at:
>>
>> http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6559067+6559086+6605100/ 
>>
>>
>> There are lots of changes, so a description of them is in order:
>>
>> * LocalRepository and URLRepository suffered from entropy, so I moved 
>> code around and brought them up to coding standards (I'm sure some 
>> things are still missed in that regard).
>>
>> * In discussion, Kumar, Stanley, and I decided that the configuration 
>> of those 2 repositories should be via system properties and the 
>> Map<String, String> given when they're constructed.  The 
>> repository-specific system propertes are read once, when the class is 
>> loaded.  Their values *override* any that could be given at 
>> construction.  BasicLauncherTests was changed to use overrides via 
>> system properties; Local and URL repository tests use overrides via 
>> the Map<String, String>.
>>
>> * When one of those repositories is created, it is OK if its source 
>> location does not exist.  In this case, when find() or list() 
>> operations are performed, they'll return empty lists.  If reload() is 
>> performed, and at that time the source location *does* exist, then 
>> subsequent find() or list() operations will return modules now 
>> available.  This is verified in the Local and URL repository tests.  
>> Checks for the prior, opposite behavior were removed from these and 
>> from RepositoryTest.
>>
>> * However, we provide a system property for each of these repository 
>> types that can cause construction/initialization to require that the 
>> source location *does* exist at that time; if not an IOException gets 
>> thrown. LocalRepositoryTest verifies this.
>>
>> * shutdown() and reload() are implemented for both repositories, and 
>> are verified in Local and URL repository tests.
>>
>> * For these repository implementations: once an instance has been 
>> initialized, subsequent initialize() invocations are a no-op.  Once an 
>> instance has been initialized and then shutdown, subsequent shutdown() 
>> invocations are a no-op.  But if an instance is initialized, then 
>> shutdown, a subsequent initialize() invocation throws 
>> IllegalStateException.  Local and URL repository tests were updated to 
>> check this.
>>
>> * RepositoryConfig now supports configuration via RepositoryFactory 
>> objects. See src/share/lib/module/repository.properties and 
>> src/share/classes/sun/module/repository/ExtensionRepositoryFactory for 
>> an example.  A new test was added for factories.
>>
>> Thanks,
>>     Dave
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev