[modules-dev] Review request for 6578988

[Stanley M. Ho]

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