[rfc][icedtea-web] intorducing connection factory

[Andrew Azores]

On 10/17/2014 09:26 AM, Jiri Vanek wrote:
> As https patch as is, is probably no go, still a lot of patching can be
> expected in this direction.
>
> During the creating of https proobing, the connection factory with
> possibility to queue individual downloads appeared to be very useful, so
> I would like to merge it to main codebase.

I'd also like more details on the specific purpose of this patch, but it 
seems well-intended enough, anyway.

Nits on the patch:

>                  result = conn.getPermission();
> -                if (conn instanceof HttpURLConnection) {
> -                    ((HttpURLConnection) conn).disconnect();
> -                }
> +                 ConnectionFactory.getConnectionFactory().disconnect(conn);
>              } catch (java.io.IOException ioe) {

>              // explicitly close the URLConnection.
> -            if (connection instanceof HttpURLConnection) {
> -                ((HttpURLConnection) connection).disconnect();
> -            }
> +           ConnectionFactory.getConnectionFactory().disconnect(connection);
>          } catch (Exception ex) {

>          } catch (PrivilegedActionException pae) {
>              throw (IOException) pae.getException();
>          } finally{
> -            if (conn instanceof HttpURLConnection) {
> -                ((HttpURLConnection) conn).disconnect();
> -            }
> +           ConnectionFactory.getConnectionFactory().disconnect(conn);
>          }

ConnectionFactory.getConnectionFactory() line indents are all having 
off-by-one errors ;)

ConnectionFactory class:
- license header says 2008

> +
> +/**
> + *
> + * @author jvanek
> + */

:)

> +//                while (!httpsConnections.isEmpty()) {
> +//                    try {
> +//                        Thread.sleep(100);
> +//                    } catch (InterruptedException ex) {
> +//                        throw new IOException(ex);
> +//                    }
> +//                }

Remove please...

Should openHttpsConnection and closeHttpsConnection really be using the 
"synchronized" keyword? It looks to me like maybe 
synchronized(httpsConnections) might be more suitable.

Why is closeHttpsConnection public if disconnect is also public and for 
https connections, simply calls through to closeHttpsConnection?

What is disconnect expected to do for URLConnections which are not 
HttpURLConnections or HttpsURLConnections? What if I have a URL with 
'jar:' protocol which I give to openConnection... this connection of 
type JarURLConnection is generated by the factory, but the factory can't 
clean it up for me?

> +            URLConnection uRLConnection = httpsConnections.get(i);

This is a funnily-named variable :) 'urlConnection' maybe?

Can the exact same URLConnection object (same reference even) really be 
in the httpsConnection list more than once? I haven't checked/tested, 
but surely opening a connection on the same URL twice returns two 
different objects. I think either the loop in closeHttpsConnections can 
either have a 'break' statement added once the matching connection is 
found, or httpsConnections can maybe be a Set rather than a List?

Thanks,
-- 
Andrew Azores