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

[Chris Hegarty]

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 ).

> Using the plural reads better given what follows:
>
>
>   272      *     class loader. The ordering that providers are located is
>   273      *     implementation specific, and an implementation is free to cache the
>   274      *     located providers. A {@linkplain java.util.ServiceConfigurationError
>   275      *     ServiceConfigurationError}, {@code Error} or {@code RuntimeException}
>   276      *     thrown from the {@code createURLStreamHandler}, if encountered, will
>   277      *     be propagated to the calling thread. The {@code
>   278      *     createURLStreamHandler} method of each provider, if instantiated, is
>   279      *     invoked, with the protocol string, until a provider returns non-null,
>   280      *     or all providers have been exhausted.
>
>
>
> In getURLStreamHandler method:
>
> 1313         if (handler == null) {
> 1314             // Try the built-in protocol handler
> 1315             handler = defaultFactory.createURLStreamHandler(protocol);
> 1316         }
> 1317
> 1318         synchronized (streamHandlerLock) {
> 1319
> 1320             URLStreamHandler handler2 = null;
> 1321
> 1322             // Check again with hashtable just in case another
> 1323             // thread created a handler since we last checked
> 1324             handler2 = handlers.get(protocol);
> 1325
> 1326             if (handler2 != null) {
> 1327                 return handler2;
> 1328             }
> 1329
> 1330             // Check with factory if another thread set a
> 1331             // factory since our last check
> 1332             if (!checkedWithFactory) {
> 1333                 handler2 = handlerFromSettableFactory(protocol);
> 1334             }
> 1335
> 1336             if (!(handler2 == null || handler2 == NULL_HANDLER)) {
> 1337                 // The handler from the factory must be given more
> 1338                 // importance. Discard the default handler that
> 1339                 // this thread created.
> 1340                 handler = handler2;
> 1341             }
> 1342
> 1343             // Insert this handler into the hashtable
> 1344             if (handler != null) {
> 1345                 handlers.put(protocol, handler);
> 1346             }
> 1347
> 1348         }
>
>
> 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?

-Chris.


> Paul.
>