RFR 8064924: Update java.net.URL to work with modules

[Paul Sandoz]

On Feb 2, 2015, at 12:34 PM, Chris Hegarty <chris.hegarty at oracle.com> wrote:

> On 02/02/15 11:00, Paul Sandoz wrote:
>> 
>> On Jan 30, 2015, at 10:10 PM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>> 
>>> On 30/01/2015 15:35, Chris Hegarty wrote:
>>>> :
>>>> 
>>>> Update webrev and spec diffs:
>>>>    http://cr.openjdk.java.net/~chegar/8064924/01/
>>>> 
>>> I think the javadoc looks much better now, thanks.
>>> 
>> 
>> Minor comments in URL:
>> 
>> Java doc on URL constructor:
>> 
>>  269      * <li>If the previous step fails to find a protocol handler, then the
>>  270      *     {@linkplain java.util.ServiceLoader ServiceLoader} mechanism is used
>>  271      *     to locate a {@code URLStreamHandlerFactory} provider using the system
>> 
>> "to locate {@code URLStreamHandlerFactory} providers using..."
> 
> Thanks Paul, Updated in the latest version ( see other mail ).
> 

Ok.


>> 
>> The code might read better if you place the stuff above the synchronized block within it (assuming that causes no issues):
>> 
>>   if (!checkedFactory) {
>>     handle = handlerFromSettableFactory(protocol);
>>     if (handle == NULL_HANDLER) handle = null;
>>   }
>>   if (handle == null) {
>>     handler = defaultFactory.createURLStreamHandler(protocol);
>>   }
>>   if (handle != null) {
>>     handlers.put(protocol, handler);
>>   }
> 
> That is a possibility, but great effort has been put into this area recently, by Peter, to attempt to keep the lookup of handlers lock free ( typically a volatile read, only requiring the lock when updating the cache ).
> 
> We would also not want to hold the lock while performing the service lookup, in which case we may end up with two synchronization blocks, so as to keep the service lookup as lazy as possible. Or have I missed our point?
> 

It was specifically to do with the optimistic call to defaultFactory.createURLStreamHandle the result of which might be thrown away, but probably not. The method handlerFromSettableFactory is anyway called from within the synchronized so it did not seem a big deal to pull in default factory call and simplify the logic.

Paul.