[modules-dev] [Fwd: [Fwd: Review request: 6563535, URLRepository.install]]

[Dave Bristor]

Stanley M. Ho wrote:
> 
> Looks good. Comments inline:
> 
> - src/share/classes/sun/module/repository/RepositoryContents.java
> 
> 83             if (m.hashCode() == mai.hashCode()) {
> 
> It is unclear why you want to compare hashcode instead of simply using 
> equals().

Because ModuleArchiveInfo.equals() is asymmetric with respect to 
ModuleArchiveInfo.hashCode().  The former is based on equality of references 
and the latter on equality of hashcodes of a a subset of the instance's 
fields.  Here's the use case that necessitates this: From JRepo's uninstall 
command, URLRepository.uninstall(ModuleArchiveInfo) is given an instance that 
is created based on information from the command line, therefore the 
ModuleArchiveInfo instance is not the one returned by an earlier 
URLRepository.install().

I added a comment in getMatch().

> - src/share/classes/sun/module/repository/URLRepository.java
> 
>>> 688             contents.put(mai, installedMD);
>>>
>>> If exception is thrown in the try/catch block, the contents will be 
>>> updated incorrectly. Even contents is updated again in the catch 
>>> block, there is still a small window that contents does not represent 
>>> the correct information. Instead, this should be updated in a finally 
>>> block.
>>
>> If it's in the finally block, it will always get updated.  But if an 
>> exception is thrown, then we don't want it to get updated, which is 
>> what we need.
>>
>> Or do you mean there should be a try/catch within the catch of the 
>> IOException, so that contents.remove(mai) in a finally?  I made this 
>> change; please let me know if it meets your intent.
> 
> I meant that the contents should not be updated at all until the entire 
> install operation succeeds. Otherwise, if a thread calls into 
> Repository.list() in the meantime, it will get a list of 
> ModuleArchiveInfo but some of the ModuleArchiveInfo might not yet been 
> installed successfully (and the installation might actually fail).

Now I understand.  Fixed, hopefully better, let me know what you think; I 
updated the webrev:
     http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6563535-b/

> 
> - Stanley