[rfc][icedtea-web] Check online detection even if checked before
Jie Kang
jkang at redhat.com
Tue Jan 27 17:09:06 UTC 2015
----- Original Message -----
> On 01/27/2015 05:01 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 01/26/2015 04:32 PM, Jie Kang wrote:
> >>>
> >>>
> >>> ----- Original Message -----
> >>>> On 01/21/2015 10:28 AM, Jiri Vanek wrote:
> >>>>> On 01/06/2015 09:19 PM, Jacob Wisor wrote:
> >>>>>> On 01/06/2015 at 01:25 PM Jacob Wisor wrote:
> >>>>>>> On 01/05/2015 at 05:19 PM Jie Kang wrote:
> >>>>>>>> Hello,
> >>>>>>>>
> >>>>>>>> This patch slightly modifies JNLPRuntime's online detection function
> >>>>>>>> to
> >>>>>>>> always perform the check, even if it has been called before.
> >>>>>
> >>>>> As Jacob is mentioning, this really is causing performance overkill.
> >>>>>>>>
> >>>>>>>> Previously, if the boolean onlineDetected wasn't null, the check
> >>>>>>>> would
> >>>>>>>> be
> >>>>>>>> skipped. This caused a bug when downloading multiple resources.
> >>>>>
> >>>>> The idea of the check was, that first resource to be downloaded, is
> >>>>> mostly
> >>>>> the most crucial - jnlp,
> >>>>> jnlpHref, mainclass, jar with main class... So this one have to be
> >>>>> downlaoded - if it is not, then
> >>>>> we can assume the network inaccessible.
> >>>>>>>>
> >>>>>>>> For each resource being downloaded, we perform the detection check.
> >>>>>>>> For
> >>>>>>>> two
> >>>>>>>> consecutive resources: If for the first resource, we detected
> >>>>>>>> offline,
> >>>>>>>> the
> >>>>>>>> boolean onlineDetected would be set to false. Then for the second
> >>>>>>>> resource,
> >>>>>>>> the detection check would be skipped, even if that resource was able
> >>>>>>>> to
> >>>>>>>> be
> >>>>>>>> connected to. This would result in the second resource being unable
> >>>>>>>> to
> >>>>>>>> be
> >>>>>>>> downloaded, even though it definitely could and should be. The patch
> >>>>>>>> fixes
> >>>>>>>> this.
> >>>>>
> >>>>> Do you have some example of this behavior causing any real issues?
> >>>>> Maybe just sync the method or otherwise ensure that while isOnline is
> >>>>> null,
> >>>>> only one download can be
> >>>>> done?
> >>>>>
> >>>>>>>>
> >>>>>>>> Thoughts?
> >>>>>>>
> >>>>>>> Although I did not test it, seems to be working as advertised.
> >>>>>>>
> >>>>>>> While looking at the code, I was wondering why
> >>>>>>> JNLPRuntime.onlineDetected
> >>>>>>> was
> >>>>>>> a Boolean object instead of a primitive. And, apart from that, the
> >>>>>>> whole
> >>>>>>> offline detection mechanism seems overly complex and unreliable to
> >>>>>>> me.
> >>>>>>> I am also not really convinced that
> >>>>>>>
> >>>>>>> public static void detectOnline(URL location) {
> >>>>>>> if (onlineDetected != null) {
> >>>>>>> return;
> >>>>>>> }
> >>>>>>> try {
> >>>>>>> if (location.getProtocol().equals("file")) {
> >>>>>>> return;
> >>>>>>> }
> >>>>>>> //Checks the offline/online status of the system.
> >>>>>>> InetAddress.getByName(location.getHost());
> >>>>>>> } catch (UnknownHostException ue) { […]
> >>>>>>>
> >>>>>>> is the best or most reliable way in J2SE to test for the
> >>>>>>> online/offline
> >>>>>>> status
> >>>>>>> of a system. Do systems having no DNS server configured or which is
> >>>>>>> unreachable
> >>>>>>> properly resolve their name? And, even if they do, does this really
> >>>>>>> actually
> >>>>>>> prove them being online?
> >>>>>>>
> >>>>>>> Now, I was considering a few different options to reliably detecting
> >>>>>>> a
> >>>>>>> system's
> >>>>>>> online/offline status. Initially, I was thinking about pinging the
> >>>>>>> default
> >>>>>>> gateway. However, hosts with public IPs may have their public IP as
> >>>>>>> the
> >>>>>>> default
> >>>>>>> gateway configured. So, the most reliable way I came up with is to
> >>>>>>> send
> >>>>>>> an echo
> >>>>>>> broadcast (on the local subnet) request. Of course, this does not
> >>>>>>> prove
> >>>>>>> the
> >>>>>>> system being connected to the /public/ internet. But this is not
> >>>>>>> neccessary
> >>>>>>> either because JNLP applications may be (which they often are) served
> >>>>>>> on
> >>>>>>> an
> >>>>>>> intranet only. So, would you mind investigating this further?
> >>>>>
> >>>>> Before doing this looks-really-stupid
> >>>>> InetAddress.getByName(location.getHost()); i was elaborating
> >>>>> on this, and have nnot found a generic way how to veryfi online status
> >>>>> from
> >>>>> java.
> >>>>>>
> >>>>>> Oh, I forgot: I am sure we do not want to make a DNS request for every
> >>>>>> resource
> >>>>>> we need to download, even when subsequent DNS resolution requests are
> >>>>>> pulled
> >>>>>
> >>>>> yah, to much correct.
> >>>>>> from the local DNS cache. So, although this patch fixes the bug, it is
> >>>>>> very
> >>>>>> inefficient and impractical because it induces an avoidable
> >>>>>> performance
> >>>>>> penalty.
> >>>>>> In other words, the online detection *logic* itself needs to be fixed.
> >>>>>>
> >>>>>> @Jie Kang:
> >>>>>> Thank you for spotting this bug.
> >>>>>>
> >>>>>
> >>>>> Is it really an bug? It actually eliminates the benefits of offline
> >>>>> environment.
> >>>>>
> >>>>> >>> consecutive resources: If for the first resource, we detected
> >>>>> >>> offline,
> >>>>> >>> the
> >>>>> >>> boolean onlineDetected would be set to false. Then for the
> >>>>> >>> second
> >>>>> >>> resource,
> >>>>> >>> the detection check would be skipped, even if that resource was
> >>>>> >>> able
> >>>>> >>> to be
> >>>>> >>> connected to. This would result in the second resource being
> >>>>> >>> unable
> >>>>> >>> to
> >>>>> >>> be
> >>>>> >>> downloaded, even though it definitely could and should be. The
> >>>>> >>> patch
> >>>>> >>> fixes
> >>>>> >>> this.
> >>>>>
> >>>>> Thinking about it - how it is possible that first resource could not be
> >>>>> downloaded, but second can
> >>>>> be, and how it is possible that first one can be missed, while second
> >>>>> one
> >>>>> not?
> >>>>>
> >>>>>
> >>>>> Maybe before deciding that environment i onnline/offline - we may try
> >>>>> to
> >>>>> download two or three
> >>>>> resources?
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> One more thing crossed my mind - if the check is done everytime
> >>>> ressource
> >>>> is
> >>>> download.
> >>>>
> >>>> Then the output of isOnline() method is actually prety undetermined.
> >>>>
> >>>>
> >>>> ?
> >>>
> >>> Hello,
> >>>
> >>>>> Thinking about it - how it is possible that first resource could not be
> >>>>> downloaded, but second can
> >>>>> be, and how it is possible that first one can be missed, while second
> >>>>> one
> >>>>> not?
> >>>>>
> >>>
> >>> This can occur in edge incidents such as:
> >>>
> >>> Resource 1 : Can't download, but it is in cache so we use the cache one.
> >>>
> >>> Resource 2 : Not in cache, but we can download, so we download it into
> >>> cache.
> >>>
> >>> Applet now works.
> >>>
> >>>
> >>> However, with the online detection as it is, the edge cases such as above
> >>> will fail.
> >>>
> >>>>> Is it really an bug? It actually eliminates the benefits of offline
> >>>>> environment.
> >>>
> >>> In terms of the offline environment, this should really only be
> >>> controlled
> >>> by the isOfflineForced setting rather than the isOnline setting. For any
> >>> resource located off-site, the only way you know if that resource is
> >>> accessible or not is by attempting to access it. The idea of an 'online'
> >>> state is flawed in this regard. The idea of an 'offline' state is fine
> >>> though. If the user sets it to offline, the rules are simple, we don't
> >>> make any connection attempts. Otherwise, we should always be attempting
> >>> to
> >>> access them.
> >>>
> >>> The more I think about this, I feel like we shouldn't be using isOnline()
> >>> to check when trying to download a resource. We should just try to
> >>> download it unless offlineIsForced. If it fails to download, the code
> >>> already deals with it as safely as possible. The online check beforehand
> >>> isn't really beneficial here.
> >>>
> >>>
> >>> Regards,
> >>>
> >>>>
> >>>>
> >>>> J.
> >>>>
> >>>
> >>
> >>
> >> Ok. Now I understand better.
> >>
> >>
> >> This should happen only if javaws was killed during downloading. And then
> >> clearing the cache is the
> >> cure.
> >>
> >>
> >> But I see your point.
> >>
> >>
> >>
> >> 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?
>
> > 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.
> >
> > 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?
>
> >
> > 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 :\
>
>
> 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, 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.
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.
Regards,
>
> J.
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
More information about the distro-pkg-dev
mailing list