[rfc][icedtea-web] fixfor portalbank.no

Adam Domurad adomurad at redhat.com
Tue Apr 30 12:28:56 PDT 2013


On 04/30/2013 10:24 AM, Jiri Vanek wrote:
> On 04/29/2013 06:15 PM, Omair Majid wrote:
>> On 04/29/2013 12:03 PM, Jiri Vanek wrote:
>>> Hi!
>>>
>>> Today I accidentally stumbled accross another regression in HEAD
>>> portalbank.no donot implements HTTP requests but provides desired jars
>>> on GET.
>>> So I made (findBestUrl) less strict.
>>
>> The change itself looks okay to me. We are dealing with more-or-less
>> misconfigured servers here, so I guess we should not be too concerned
>> about optimizing this case. A fallback to GET seems appropriate.
>>
>>> * net/sourceforge/jnlp/cache/ResourceTracker : (findBestUrl)
>>>    now trying GET after each error request of HEAD
>>
>> Please complete the ChangeLog ;)
>
> ups:)
>
>
> * net/sourceforge/jnlp/cache/ResourceTracker : (findBestUrl)
> now trying GET after each error request of HEAD type. Changed and
> added debug messages. (getUrlResponseCode) closing of stream
> moved to separate method HttpUtils.consumeAndCloseConnectionSilently
> * net/sourceforge/jnlp/util/HttpUtils.java: new file designed  for
> http utils. Now contains (consumeAndCloseConnection) and
> (consumeAndCloseConnectionSilently) which calls consumeAndCloseConnection
> but do not rethrow exception
> * netx/net/sourceforge/jnlp/util/StreamUtils.java: removed 
> (consumeAndCloseInputStream)
> now improved and moved to HttpUtils
>
>
> tests:
>
> * netx/net/sourceforge/jnlp/cache/Resource.java: added fixme to warn 
> before wrong url comparator
> * netx/net/sourceforge/jnlp/Version.java: removed useless main. Its 
> purpose moved to new
> * tests/netx/unit/net/sourceforge/jnlp/VersionTest: some small tests 
> to version class
> * tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java: 
> added tests to
> (getUrlResponseCode) and (findBestUrl)
> * tests/netx/unit/net/sourceforge/jnlp/util/HttpUtilsTest.java: added 
> tests for
> (consumeAndCloseConnectionSilently) and (consumeAndCloseConnection)
> * tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest: added 
> license header
> * tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java: and
> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java: added 
> support for simulation
> of not working HEAD request
>
>
>
>>
>>> @@ -910,7 +911,7 @@
>>>
>>>                   int responseCode = getUrlResponseCode(url, 
>>> requestProperties, "HEAD");
>>>
>>> -                if (responseCode == 
>>> HttpURLConnection.HTTP_NOT_IMPLEMENTED ) {
>>> +                if (responseCode < 200 || re sponseCode >= 300) {
>>>                       System.err.println("NOTE: The server does not 
>>> appear to support HEAD requests, falling back to GET requests.");
>>
>> The error message is not very accurate now. We are going through a list
>> of URLs, searching for the best one. It's likely the URLs will not exist
>> and so we will probably get 404 errors. The message can be very
>> misleading in that case.
>
> Ok. I have improved the message.
>>
>>> +public class HttpUtils {
>>
>> Could you add a unit test for this?
>
> Ok, I have at the end added unittests to all methods I have touched. 
> Imho it is worthy - but no more bugs was found this time :)
>
> If you do not like the tests,please let me push the fix and discuss 
> the tests separately.
>
> Also do you mind you can check - Re: [rfc] [icedtea-web] renaming 
> Messages_cs_CZ to Messages_cs (was Re:ITW tranlsation) ? It should 
> just need and experience yes/now. The code change itself should be 
> very simple then (just renaming the file..)
>>
>> Cheers,
>> Omair
>>
>

> diff -r e34db561b7b9 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java    Mon Apr 
> 29 16:24:37 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java    Tue Apr 
> 30 16:12:59 2013 +0200
> @@ -24,14 +24,10 @@
>  import java.io.IOException;
>  import java.io.InputStream;
>  import java.io.OutputStream;
> -import java.io.UnsupportedEncodingException;
>  import java.net.HttpURLConnection;
>  import java.net.MalformedURLException;
> -import java.net.URI;
> -import java.net.URISyntaxException;
>  import java.net.URL;
>  import java.net.URLConnection;
> -import java.net.URLDecoder;
>  import java.security.AccessController;
>  import java.security.PrivilegedAction;
>  import java.util.ArrayList;
> @@ -49,7 +45,7 @@
>  import net.sourceforge.jnlp.event.DownloadEvent;
>  import net.sourceforge.jnlp.event.DownloadListener;
>  import net.sourceforge.jnlp.runtime.JNLPRuntime;
> -import net.sourceforge.jnlp.util.StreamUtils;
> +import net.sourceforge.jnlp.util.HttpUtils;
>  import net.sourceforge.jnlp.util.UrlUtils;
>  import net.sourceforge.jnlp.util.WeakList;
>
> @@ -859,7 +855,7 @@
>       * @return the response code if HTTP connection, or 
> HttpURLConnection.HTTP_OK if not.
>       * @throws IOException
>       */
> -    private static int getUrlResponseCode(URL url, Map<String, 
> String> requestProperties, String requestMethod) throws IOException {
> +     static int getUrlResponseCode(URL url, Map<String, String> 
> requestProperties, String requestMethod) throws IOException {
>          URLConnection connection = url.openConnection();
>
>          for (Map.Entry<String, String> property : 
> requestProperties.entrySet()){
> @@ -874,7 +870,7 @@
>
>              /* Fully consuming current request helps with connection 
> re-use
>               * See 
> http://docs.oracle.com/javase/1.5.0/docs/guide/net/http-keepalive.html */
> - StreamUtils.consumeAndCloseInputStream(httpConnection.getInputStream());
> + HttpUtils.consumeAndCloseConnectionSilently(httpConnection);
>
>              return responseCode;
>          }
> @@ -891,7 +887,7 @@
>       * @param resource the resource
>       * @return the best URL, or null if all failed to resolve
>       */
> -    private URL findBestUrl(Resource resource) {
> +     URL findBestUrl(Resource resource) {
>          DownloadOptions options = downloadOptions.get(resource);
>          if (options == null) {
>              options = new DownloadOptions(false, false);
> @@ -910,8 +906,9 @@
>
>                  int responseCode = getUrlResponseCode(url, 
> requestProperties, "HEAD");
>
> -                if (responseCode == 
> HttpURLConnection.HTTP_NOT_IMPLEMENTED ) {
> -                    System.err.println("NOTE: The server does not 
> appear to support HEAD requests, falling back to GET requests.");
> +                if (responseCode < 200 || responseCode >= 300) {
> +                    System.err.println("NOTE: The server has returned 
> " + responseCode + " code for HEAD request for " + 
> url.toExternalForm() + ". IcedTea-Web will fail back to GET.");
> +                    System.err.println("NOTE: it is possible that 
> server is just not supporting  HEAD request, but more likely the file 
> is really invalid or missing");

This is a poor error message, please remove it.
Or if you wish, you may keep the old message in the case that 
'responseCode == HttpURLConnection.HTTP_NOT_IMPLEMENTED'. In that case 
the server is telling us they do not support HEAD requests.
Actually, this method would be a bit better (and not require 2 
connections per failure) if the outer loop was:

/* Use GET request as a fallback in the rare case the server does not 
support HEAD requests */
final String requestMethods = {"HEAD", "GET"};
for (String requestMethod : requestMethods) {
         for (URL url : urls) {
             ....snip...
                 int responseCode = getUrlResponseCode(url, 
requestProperties, requestMethod);
             ....snip...

Then we need not fall-back inside the loop at all. The logic would 
remain pretty much the same.
In this case we can still explicitly check for ==NOT_IMPLEMENTED in 
which case we will report '"Server does not support " + requestMethod'. 
(Not supporting GET would really be something special though :-)

I am in favour of this variation (Omair seemed to prefer it too :-). It 
guarantees the largest benefit from HEAD requests in the case of 
trial-and-error jar name guesses, for just a little bit of restructuring.

>                      /* Fallback: use GET request in the rare case the 
> server does not support HEAD requests */
>                      responseCode = getUrlResponseCode(url, 
> requestProperties, "GET");
>                  }
> @@ -922,6 +919,10 @@
>                          System.err.println("best url for " + 
> resource.toString() + " is " + url.toString());
>                      }
>                      return url; /* This is the best URL */
> +                } else {
> +                    if (JNLPRuntime.isDebug()) {
> +                        System.err.println(resource.toString() + "'s 
> url " + url.toString()+" returned "+responseCode);
> +                    }

OK.

>                  }
>              } catch (IOException e) {
>                  // continue to next candidate
> diff -r e34db561b7b9 netx/net/sourceforge/jnlp/util/HttpUtils.java
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/HttpUtils.java    Tue Apr 30 
> 16:12:59 2013 +0200
> @@ -0,0 +1,71 @@
> +/*
> + Copyright (C) 2011 Red Hat, Inc.
> +
> + This file is part of IcedTea.
> +
> + IcedTea is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as 
> published by
> + the Free Software Foundation, version 2.
> +
> + IcedTea is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with IcedTea; see the file COPYING.  If not, write to
> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, 
> Boston, MA
> + 02110-1301 USA.
> +
> + Linking this library statically or dynamically with other modules is
> + making a combined work based on this library.  Thus, the terms and
> + conditions of the GNU General Public License cover the whole
> + combination.
> +
> + As a special exception, the copyright holders of this library give you
> + permission to link this library with independent modules to produce an
> + executable, regardless of the license terms of these independent
> + modules, and to copy and distribute the resulting executable under
> + terms of your choice, provided that you also meet, for each linked
> + independent module, the terms and conditions of the license of that
> + module.  An independent module is a module which is not derived from
> + or based on this library.  If you modify this library, you may extend
> + this exception to your version of the library, but you are not
> + obligated to do so.  If you do not wish to do so, delete this
> + exception statement from your version.
> + */
> +package net.sourceforge.jnlp.util;
> +
> +import java.io.IOException;
> +import java.io.InputStream;
> +import java.net.HttpURLConnection;
> +
> +public class HttpUtils {
> +
> +    /**
> +     * Ensure a HttpURLConnection is fully read, required for correct 
> behaviour
> +     * in some APIs, namely HttpURLConnection.
> +     */

This message is confusing. Please drop the last part ('in some APIs, 
namely HttpURLConnection'). Please duplicate this comment for 
'consumeAndCloseConnection', and additionally indicate for 
'consumeAndCloseConnection' that IOException's are caught and printed.

> +    public static void 
> consumeAndCloseConnectionSilently(HttpURLConnection c) {
> +        try {
> +            consumeAndCloseConnection(c);
> +        } catch (IOException ex) {
> +            ex.printStackTrace(System.err);
> +        }
> +    }
> +
> +    public static void consumeAndCloseConnection(HttpURLConnection c) 
> throws IOException {
> +        InputStream in = null;
> +        try {
> +             in = c.getInputStream();
> +            byte[] throwAwayBuffer = new byte[256];
> +            while (in.read(throwAwayBuffer) > 0) {
> +                /* ignore contents */
> +            }
> +        } finally {
> +            if (in!=null){
> +                in.close();
> +            }
> +        }
> +    }
> +}
> diff -r e34db561b7b9 netx/net/sourceforge/jnlp/util/StreamUtils.java
> --- a/netx/net/sourceforge/jnlp/util/StreamUtils.java    Mon Apr 29 
> 16:24:37 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/util/StreamUtils.java    Tue Apr 30 
> 16:12:59 2013 +0200
> @@ -42,22 +42,10 @@
>  import java.io.IOException;
>  import java.io.InputStream;
>  import java.io.InputStreamReader;
> +import java.net.HttpURLConnection;
>
>  public class StreamUtils {
>
> -    /**
> -     * Ensure a stream is fully read, required for correct behaviour 
> in some
> -     * APIs, namely HttpURLConnection.
> -     * @throws IOException
> -     */
> -    public static void consumeAndCloseInputStream(InputStream in) 
> throws IOException {
> -        byte[] throwAwayBuffer = new byte[256];
> -        while (in.read(throwAwayBuffer) > 0) {
> -            /* ignore contents */
> -        }
> -        in.close();
> -    }
> -
>      /***
>       * Closes a stream, without throwing IOException.
>       * In case of IOException, prints the stack trace to System.err.

Will review tests separately.

Thanks,
-Adam



More information about the distro-pkg-dev mailing list