[rfc][icedtea-web] Check online detection even if checked before

Jie Kang jkang at redhat.com
Tue Feb 10 18:49:46 UTC 2015



----- Original Message -----
> On 02/06/2015 03:22 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> ...snip...
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> If -Xoffline is specified, I would like to keep current approach.
> >>>>>>
> >>>>>>
> >>>>>> For the online detection, I'm really concerned about isOnline result
> >>>>>> (and
> >>>>>> so
> >>>>>> behaviour of system)
> >>>>>>
> >>>>>>
> >>>>>> So brainstroming (valid only if no xOffline is specified):
> >>>>>>
> >>>>>> on begging assume environment is offline (no check was done)
> >>>>>> The check on "is online" will be done always from now
> >>>>>> If suddenly the resource is being avaiable online , switch to "online
> >>>>>> verified" and consider
> >>>>>> environment is online.
> >>>>>> No more checks are done from here.
> >>>>>>
> >>>>>> with Xoffline - no check is done ever.
> >>>>>>
> >>>>>> What do you think?
> >>>>>
> >>>>> Hello,
> >>>>>
> >>>>> If I understood correctly, I think the change to make it behave like
> >>>>> above
> >>>>> would be simple:
> >>>>>
> >>>>> In JNLPRuntime:
> >>>>>        public static void detectOnline(URL location) {
> >>>>>            if (onlineDetected != null) {
> >>>>>                return;
> >>>>>            }
> >>>>>
> >>>>> change:  if (onlineDetected != null) {
> >>>>
> >>>> [*] If so, then with related Boolean -> boolean change.
> >>>
> >>> Sorry can you explain this comment a little more?
> >>
> >>
> >> If the null will be never used, then it have to type of boolean, not
> >> Boolean.
> >> And I think it will
> >> not be. the default value now will be false.
> >>
> >>    [***] I have now realized - how slow it will be if the environment
> >>    really
> >>    will be offline? It will
> >> keep "pinging" all resources, and actually dying with timeouts. ... You
> >> will
> >> have to measures this
> >> on sme biger (all will start offline with current appracha and mostly
> >> sometimes dye later with
> >> runtimeexception)
> >>
> >> There may be hiddden performance drop. (as opposite to current state,
> >> where
> >> is performance up when
> >> you plug out the cable)
> >
> > Yes, you are right. This alternative change: if (onlineDetected == true)
> >
> > will keep pinging resources and not have timeouts. I don't think it is a
> > good solution anymore.
> >
> >
> >>>
> >>>>
> >>>>>        to:  if (onlineDetected == true) {
> >>>>>
> >>>>>
> >>>>> However, I wonder if we need to make it so complicated.
> >>>>>
> >>>>> I see two situations:
> >>>>>
> >>>>> A: Xoffline : no check is done ever and we never download : I think
> >>>>> ResourceTracker.initializeResource() will need some patching for this.
> >>>>> But
> >>>>> I already wrote one, just haven't sent it yet.
> >>>>>
> >>>>> B: 'Online' : for each resource, try online and if it fails, try cache,
> >>>>> otherwise error. This is already coded safely.
> >>>> I doubt.
> >>>>
> >>>>>
> >>>>>      My question here is: What is benefit of doing isOnline check?
> >>>>>      Where
> >>>>>      else
> >>>>>      is this used outside of initializing and downloading resources?
> >>>>
> >>>> Find usages is your friend.
> >>>>
> >>>> Especially the one in resourceTracekr is imho critical.
> >>>
> >>> Hahah, I think the opposite. The ones outside of ResourceTracker are
> >>> great,
> >>> but the one in ResourceTracker is not as good.
> >>
> >> And how will you proceed without them? I doubt it will work.
> >>>
> >>>>>
> >>>>>      When we use URLConnection object and try to initialize the
> >>>>>      resource,
> >>>>>      this already does a check for us. As well, the code already deals
> >>>>>      with
> >>>>>      the possible situations here.
> >>>>
> >>>> This is not working as smotthly as you expect. Please be careful with
> >>>> any
> >>>> fixes here, otherwise you
> >>>> will break offline capabilities ITW with effort achieved.
> >>>
> >>> Can you explain a little more? Possibly provide some examples and
> >>> describe
> >>> what you mean by offline capabilities?
> >>
> >> [**] That every part of ITW works even if you unplug the utp cable and
> >> turn
> >> of WiFi. Its crucial
> >> part of javaws implementation, which was long time (before offline patch)
> >> missing. As benefit, also
> >> appelts works offline. And it is something What I also wont to keep.
> >
> > The original patch retains this behaviour, no?
> >
> >>
> >>>
> >>>
> >>>> 	
> >>>>>
> >>>>>      I do not think the isOnline check before trying to use
> >>>>>      URLConnection
> >>>>>      object has a performance benefit that justifies the extra
> >>>>>      complexity
> >>>>>      in
> >>>>>      code.
> >>>>
> >>>> No it does not. But make life much easier.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Regards,
> >>>>
> >>>>
> >>>>
> >>>> Looking back to the orriginal patch which introduced offline
> >>>> capabilities:
> >>>>
> >>>> isOnlineDetected
> >>>> isOnline
> >>>>      and
> >>>> detectOnline
> >>>>
> >>>>
> >>>> and their's usages -
> >>>> 4 (+3 comfortable)
> >>>> 2
> >>>>      and
> >>>> 3
> >>>>
> >>>> it is not much, but the whole concept of those have to be revisited. If
> >>>> you
> >>>> grant that all the itw
> >>>> javaws and applets will work offline, than you can do whatever you need.
> >>>> But
> >>>> then I would recommand
> >>>> you to remove the offline capabilities patch, and implement the whole
> >>>> design
> >>>> on your own.
> >>>
> >>> I'd rather not remove the patch :\
> >>
> >> But it may be best way, Remove it, and try to reimplemnt the offline
> >> solution
> >> with the bug you found
> >> fixed.
> >>>
> >>>>
> >>>>
> >>>> I'm quite afraid to change individual parts of this patch. And I do not
> >>>> wont
> >>>> some functions to
> >>>> return random values.
> >>>>
> >>>>
> >>>> Even the change of yours  [* above] may have side effects.  But It may
> >>>> be
> >>>> what is seek. Fix for your
> >>>> issue, and still keeping  all the behaviours I would like to have.
> >>>
> >>> I know for sure the [*] change will fix the issue I am having. I also
> >>> strongly think that it will keep the behaviours you'd like to have,
> >>
> >> Ok. So well.. lets try it. (but be aware of [***])
> >
> > After reading [***] I realize this new fix is incorrect.
> >
> >>
> >> I would like to know Jacob's opinion about [*] [**] and [***] :)
> >>
> >>   > but this would need a bit more review. But, if you can specify exactly
> >>   > what behaviours you are
> >> looking for, then it will be easier to see if this fix works or not.
> >>>
> >>
> >> explained  in [**]
> >>
> >>> At the same time, I still feel a call to detectOnline() shouldn't return
> >>> just because it's been called before. I think the only cost of the
> >>> original patch is in performance and in my opinion it is extremely small.
> >>> It keeps pretty much everything you want, fixes my issue, and has one
> >>> drawback that I can see, which is performance. However, how big of a
> >>> performance drop is this? I don't think it's even close to noticeable.
> >
> >
> > After all this discussion, I believe the original patch is still the best
> > choice. It does retain the features you specified, it doesn't have the
> > issue of [***], and it fixes the bug.
> 
> If you can swear that offline capabilities are not broken by this (which I
> doubt a bit!) then I'm
> inclined to try your approach.
> >
> > The only issue I see with this patch is a clear performance drop. However,
> > I must ask, is this really an issue? The time it takes for an online check
> > is pretty miniscule, and on 'average' machines and networks, I would say
> > unnoticeable to the user.
> 
> I agree with you.
> >
> > If you'd like, I can create + run some tests to see how much slower it is,
> > but I feel like it will be in the millisecond range or smaller...
> >
> > I have attached the patch again for reference.
> >
> 
> The issue I still see is the starnge result of isOnline()
> 
> With your changesst, the original behavior is impossible to achieve. So even
> If I'm now convinced
> that what you do is right, it seems to be  not completed.
> 
> The methods of isOnline and isOnlineDetected are depnding on  state of
> onlineDetected and logic of
> ITW is depnding on its result.

I think there are two situations that we need to recognize.

Initializing/Downloading resources : this needs to always detect online for each resource

Everything else : this needs the logic of isOnline, isOnlineDetected as they are currently implemented.



Right now, these two situations use the same code in JNLPRuntime, but they need different behaviour.

So, I think the real solution is to modify/add code so the two situations don't use the same code. Please see [1] [rfc][icedtea-web] Refactor initializeResources in ResourceDownloader


[1] http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2015-February/030726.html


Does this make any sense to you?


> 
> My idea is:
>   - keep your changeset - always repeat detectOnline, unless xoffline is
>   specified
>   - keep onlineDetected as false - once moved to true, make it impossible to
>   set it back to false
>     - this may have side effects when network is somehow closed iin runtime,
>     but afiak this is not

Jacob pointed out connection timeouts being a possible issue and I think changing it like this will cause timeouts to occur.


Regards,

> supported even now (nor ever?)
> 
> 
> Thoughts? Actually ideas how it will behave?
> 
> 
> Update.... After playing with it I found that what I suggested is breaking
> file:// handling
> Something liek attached had worked for me. 9note - try tosolve it without
> looking to my "patch" I
> dont like my approach.
> 
> btw - I found a bug in my -html switch. It works offline only when Xoffline
> is specified... weird...
>   J.
> 
> 
> 
> 
> 
> 
> 
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern


More information about the distro-pkg-dev mailing list