[rfc][icedtea-web] intorducing connection factory

[Jiri Vanek]

On 10/20/2014 04:52 PM, Andrew Azores wrote:
> 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 ;)
>

sure
> ConnectionFactory class:
> - license header says 2008

sure
>
>> +
>> +/**
>> + *
>> + * @author jvanek
>> + */
>
> :)
>

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

actually not. It should be uncommented.

See that the if clause poinitng to this  is always false.
>
> Should openHttpsConnection and closeHttpsConnection really be using the "synchronized" keyword? It
> looks to me like maybe synchronized(httpsConnections) might be more suitable.

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


should be not. Will be fixed as private..
>
> 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?

Then the factory should be teached to deal with it. Is this connection somewhere in ITW right now?
  - or different point of view - the connection factory should deal with downlaod on network 
protocols. Maybe it should be renamed to RemoteURlConnectionFactory?  Can jar be remote?

Anyway this is good catch.
>
>> +            URLConnection uRLConnection = httpsConnections.get(i);
>
> This is a funnily-named variable :) 'urlConnection' maybe?


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

No.
> 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?

Yes. I will elaborate on to get rid of any pooling  which is now present in close future. Right now 
this is approach I know about to be working.  So any replacement will need to keep this behavior.


One reason which maybe was not obvious. During debugging of https, I really needed stop-world 
bottleneck.  So any ongoing fix to https will need similar workaround as is this factory before 
actual fix may be incorporated.


What I should wrote in begging, I do not believe https issues  will disapear. And some workarounds 
will have to be added. Even if it will be custom patch for some custom build. And each this patch 
will need to have seem actions "before connect" and "after disconnect". I don't know  How to handle 
those on single place if not in similar patch as is this under review. (Actually, any better or 
simplier patch, which will provide similar functinality, is *really* welcommed)


J.