[rfc][icedtea-web] https probing

[Jiri Vanek]

On 09/24/2014 04:41 PM, Jacob Wisor wrote:
> On 09/23/2014 at 05:31 PM, Jiri Vanek wrote:
>> On 09/14/2014 06:17 AM, Jacob Wisor wrote:
>>> On 09/10/2014 at 11:42 AM, Jiri Vanek wrote:
>
> Because the discussion on this patch has become quite extensive I am going to focus on the two most
> important issues only. At least for now.
>
yy. Thank  you. And sorry for delay in reply.


>>>> +        if (url.getProtocol().equalsIgnoreCase("https") &&
>>>> isProbingAllowed()) {
>>>> +            if (isSyncForced()) {
>>>> +                while (!httpsConnections.isEmpty()) {
>>>> +                    try {
>>>> +                        Thread.sleep(100);
>>>> +                    } catch (InterruptedException ex) {
>>>> +                        throw new IOException(ex);
>>>> +                    }
>>>> +                }
>>>
>>> Polling? Really? Is this really the proper way to do it? And, do we really
>>> need to offer
>>
>>
>> Actually yes. I dont know about better way here. I will be happy if you have some.
>>
>> What I wonted to take care about, is situatuion when the individual https
>> settings differe.   Then the first one must be disconnected, before second can
>> apply for new settings.
>> And so tehre is need to not accept other https conenctions until previous one
>> disconnects.
>>
>>
>>> synchronous connections?
>>> Besides, what about infinite looping?
>>
>> Do you mean that it can deadlock here? Yes. I'm thinking the same. But The same
>> deadlock will rise now too. For my pooling here, the timeout is most simple
>> solution and will probably appear in some next iteration changeset
>
> I think you cannot avoid to pay tribute to the fact that networks are inherently asynchronous by
> their very nature (I am not talking about USB here ;-) ). Having said that, we /can/ support
> synchronous connection creating but it should be based on asynchronous calls because ... you guessed
> it, of a network's inherent asynchronous nature.

Yes. But it does not need to prevent us from connect, wait, disconnect... . If the internal impl is 
asinc, then let its be.
> Anyway, the Http(s)Factory should support creating HTTP /and/ HTTPS connections (and thus download

Why to include http into the synchronized/probed way?  The properties I'm touching, and for which 
I'm forcing the syncs, are affecting https only.

> JNLP resources) not only asynchronously but also concurrently. None of the protocols posses a
> property that prevents us from implementing asynchronous connection creating, forces us to support
> synchronous connection creating only (even when testing for multiple versions), nor imposes a limit
> on the full (logical) potential of a network. Besides, establishing connections synchronously only,
> is definitely not the state of the art for the 21st century. ;-)

Yes. But may be a good workaround for few rare cases.
>
> Also, please note that your current approach is not thread-safe, even for synchronous calls only!
> Thread-safety is actually a MUST for a Http(s)Factory. Simply putting the synchronized keyword in
> front of a method declaration is not fully solving the problem here nor should actually be done this
> way. What you actually want to synchronize is the access to the Http(s)Factory's internal state, not
> the access to the factory (its methods) itself. There is no need to block *all* other threads which
> want to create a connection just because one thread is using the factory or is waiting for the
> factory to complete creating a connection for it. I strongly advise you to rewrite the factory
> accordingly.
>
> The proper way to provide a synchronous call in an asynchronous environment is to block or yield.
> And, you certainly do not accomplish this with potentially infinite loops making the current thread
> wait an arbitrary amount of time. What you do is; you set off the asynchronous call and then wait
> (Object.wait()) until you get notified (Object.notify()) by the asynchronous call's on complete method.

Hmm. Will it still allows me to sync on https only? If so, then yes, it would be best solution and I 
was afraid of it in vain.
>
>>>> [...]
>>>> +        //this s intentional search by object value. equals do not work
>>>> +        for (int i = 0; i < httpsConnections.size(); i++) {
>>>
>>> Start from the end, decrement to 0, and compare to 0. Comparing to 0 gives
>>> almost always far better
>>> performance on modern processors because they are heavily optimized for branch
>>> prediction on 0.
>>>
>>>> +            URLConnection uRLConnection = httpsConnections.get(i);
>>>> +            if (uRLConnection == conn) {
>>>> +                httpsConnections.remove(i);
>>>> +                i--;
>>>
>>> Besides, why not use ArrayList.removeAll(Collection) here? It is probably much
>>> faster, even though
>>> you have to create a Collection instance.
>>
>>
>> I'm not sure what you mean here.
>
> You are trying to remove all "conn" elements from the "httpsConnections" ArrayList the awkward way.
> The best way to do this is to call ArrayList.removeAll(Collection) instead of iterating through the
> ArrayList manually.

As the comment says - I can not use remove all, because it is removing via equals. And I need to 
remove based on references.
>
>>>> [...]
>>>> +        public static URLConnection openConnection(URL url, boolean ssl2,
>>>> boolean insecure)
>>>> throws IOException {
>>>> +            try {
>>>> +                if (ssl2) {
>>>> +                    System.setProperty("https.protocols", "SSLv3,SSLv2Hello");
>>>> +                } else {
>>>> +                    System.clearProperty("https.protocols");
>>>> +
>>>> +                }
>>>
>>> I am absolutely sure we do not want to modify *system* properties for *all*
>>> future connections here.
>>> What about other existing and other future connections, especially connections
>>> created by a JNLP
>>> application?
>>> And then there are SecurityManager implications. What if these or all system
>>> properties are read-only?
>>> So no, we do not want to modify system properties at runtime.
>>
>> Ok.I made investigations and you are right. This is show stopper on current
>> approach.
>> The issue helpcrypto is facing with jdk8 have solution based on properties to:(
>
> This raises the question whether we should bother about this at all, since the SSL version of HTTPS
> connections can and have always been configurable by system properties in the JRE. :-D

I understand that. On opposite - you must agree that pass property to javaws soemtimes need some 
skill and I  fo not expect this to be done by plain user. So why itw should not try few most common 
cases on its own? And what about case when each https connection needs different settings?
>
>> By default, no probing will be done, and  itw will work as before.
>> There will be only one property - public static final String
>> KEY_HTTPS_PROBINGALLOWED = "deployment.security.https.probing.allowed";
>
> Then this property should be renamed to "deployment.security.https.probing.enabled".
>
>> By default false, and with possible true/false
>>
>> Maybe probing will be possible to turn on in runtime, if https connection fails,
>> on users approve.
>
> It could. But, on the other hand who or what would turn on probing for HTTPS connections at runtime?

See the fixme in code. Only one who can do this is ITW. If it fails https during *downloading* of 
*resources* then it ask if it should switch to it. Maybe with description of list of tryed methods 
or something like that.
> The JNLP application? Certainly not. A JNLP application does not even control downloading resources
> directly.

sure :)) For jnlp app, after the controll is given to it,  the properties have to be returned d in 
original state.

> Then perhaps the user? If so, this would require for the user to be able to change the desired JNLP
> application's JRE configuration in some kind of UI or command line interface provided by
> IcedTea-Web. And it would make tracing the configuration for every running JNLP application and
> visualizing system properties mandatory. This is counter intuitive and breaks JNLPRuntime's current
> concept which is to configure the JRE for a JNLP application's entire lifetime. Besides, there is
> currently no UI or infrastructure to support this in IcedTea-Web.
Oh.. nothing like that. I hope!

>
>> Once proobing is true, then the scenario will be same as attached (same as
>> always in this thread) chnageset with :
>> deployment.security.https.syncforced = true
>
> Why so? Why should creating HTTPS connections be synchronous only? There is no rational reason to
> impose such a limit. Besides, it definitely leads to increased latencies and possibly to decreased
> throughput.
>


This  last paragraphs makes me wonder if we understand each other.


I wont the sync acces only for https conenction. For https only, because I'm mdyfing https affecting 
rpoerties only and/or https  certificate management. So no need to block other protocols.

I understand with every drawback of forcing https conenctions  to be not-parallel. Bu it have to be 
done, if I wont to return the properties  to the original state after each disconnect.


J.