[rfc][icedtea-web] intorducing connection factory

[Jiri Vanek]

On 10/20/2014 05:09 PM, Jiri Vanek wrote:
> 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.

With mantioned minors fixed, I will push tomorrow.

J.