[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