# HG changeset patch # User Jiri Vanek # Date 1452073771 -3600 # Node ID 199702cfbca7128da6e8b95411b03dbff6aa923a # Parent f48d4c62c2e3347f8b7af9b4da8194a2ee9c9f61 Fixed PR2591 - IcedTea-Web request resources twice for meta informations and causes ClientAbortException on tomcat in conjunction with JnlpDownloadServlet diff -r f48d4c62c2e3 -r 199702cfbca7 ChangeLog --- a/ChangeLog Wed Jan 06 09:53:38 2016 +0100 +++ b/ChangeLog Wed Jan 06 10:49:31 2016 +0100 @@ -1,3 +1,16 @@ +2016-01-06 Jiri Vanek + + Fixed PR2591 - IcedTea-Web request resources twice for meta informations and + causes ClientAbortException on tomcat in conjunction with JnlpDownloadServlet + * NEWS: mentioned PR2591 + * netx/net/sourceforge/jnlp/cache/ResourceDownloader.java: CodeWithRedirect renamed + to UrlRequestResult and now cached also lastModified and length if available. + (initializeFromURL) now expects UrlRequestResult instead of URL, (findBestUrl) + now returns in same manner + (SimpleTest1CountRequests) now passes + * tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java: adapted + to ResourceDownloader. + 2016-01-06 Jiri Vanek Added redirection tests @@ -7,9 +20,9 @@ * tests/reproducers/simple/simpletest1/testcases/SimpleTest1Test.java: small refactoring to reuse checking methods * tests/reproducers/simple/simpletest1/testcases/SimpleTest1CountRequests.java: - added set of tests to test behavior under various redirect codes + Added FAILING tests for 2591 - counting ITW requests to test server * tests/reproducers/simple/simpletest1/testcases/SimpleTestDefaultRedirects.java: - Added FAILING tests for 2591 - counting ITW requests to test server + added set of tests to test behavior under various redirect codes 2016-01-05 Jiri Vanek diff -r f48d4c62c2e3 -r 199702cfbca7 NEWS --- a/NEWS Wed Jan 06 09:53:38 2016 +0100 +++ b/NEWS Wed Jan 06 10:49:31 2016 +0100 @@ -11,6 +11,7 @@ New in release 1.6.2 (YYYY-MM-DD): * all connection restrictions now consider also port * PR2779: html-gen.sh: Don't try to call hg if .hg directory isn't present +* PR2591 - IcedTea-Web request resources twice for meta informations and causes ClientAbortException on tomcat in conjunction with JnlpDownloadServlet * NetX - main-class attribute trimmed by default - in strict mode, main-class attribute checked for invalid characters diff -r f48d4c62c2e3 -r 199702cfbca7 netx/net/sourceforge/jnlp/cache/ResourceDownloader.java --- a/netx/net/sourceforge/jnlp/cache/ResourceDownloader.java Wed Jan 06 09:53:38 2016 +0100 +++ b/netx/net/sourceforge/jnlp/cache/ResourceDownloader.java Wed Jan 06 10:49:31 2016 +0100 @@ -57,8 +57,8 @@ * HttpURLConnection.HTTP_OK and null if not. * @throws IOException */ - static CodeWithRedirect getUrlResponseCodeWithRedirectonResult(URL url, Map requestProperties, ResourceTracker.RequestMethods requestMethod) throws IOException { - CodeWithRedirect result = new CodeWithRedirect(); + static UrlRequestResult getUrlResponseCodeWithRedirectonResult(URL url, Map requestProperties, ResourceTracker.RequestMethods requestMethod) throws IOException { + UrlRequestResult result = new UrlRequestResult(); URLConnection connection = ConnectionFactory.getConnectionFactory().openConnection(url); for (Map.Entry property : requestProperties.entrySet()) { @@ -92,6 +92,9 @@ } ConnectionFactory.getConnectionFactory().disconnect(connection); + result.lastModified = connection.getLastModified(); + result.length = connection.getContentLengthLong(); + return result; } @@ -120,7 +123,7 @@ private void initializeOnlineResource() { try { - URL finalLocation = findBestUrl(resource); + UrlRequestResult finalLocation = findBestUrl(resource); if (finalLocation != null) { initializeFromURL(finalLocation); } else { @@ -136,18 +139,25 @@ } } - private void initializeFromURL(URL location) throws IOException { + private void initializeFromURL(UrlRequestResult location) throws IOException { CacheEntry entry = new CacheEntry(resource.getLocation(), resource.getRequestVersion()); entry.lock(); try { - resource.setDownloadLocation(location); - URLConnection connection = ConnectionFactory.getConnectionFactory().openConnection(location); // this won't change so should be okay not-synchronized + resource.setDownloadLocation(location.URL); + URLConnection connection = ConnectionFactory.getConnectionFactory().openConnection(location.URL); // this won't change so should be okay not-synchronized connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip"); File localFile = CacheUtil.getCacheFile(resource.getLocation(), resource.getDownloadVersion()); - long size = connection.getContentLengthLong(); + Long size = location.length; + if (size == null) { + size = connection.getContentLengthLong(); + } + Long lm = location.lastModified; + if (lm == null) { + lm = connection.getLastModified(); + } - boolean current = CacheUtil.isCurrent(resource.getLocation(), resource.getRequestVersion(), connection.getLastModified()) && resource.getUpdatePolicy() != UpdatePolicy.FORCE; + boolean current = CacheUtil.isCurrent(resource.getLocation(), resource.getRequestVersion(), lm) && resource.getUpdatePolicy() != UpdatePolicy.FORCE; if (!current) { if (entry.isCached()) { entry.markForDelete(); @@ -168,14 +178,15 @@ resource.changeStatus(EnumSet.of(PRECONNECT, CONNECTING), EnumSet.of(CONNECTED, PREDOWNLOAD)); // check if up-to-date; if so set as downloaded - if (current) + if (current) { resource.changeStatus(EnumSet.of(PREDOWNLOAD, DOWNLOADING), EnumSet.of(DOWNLOADED)); + } } // update cache entry if (!current) { - entry.setRemoteContentLength(connection.getContentLengthLong()); - entry.setLastModified(connection.getLastModified()); + entry.setRemoteContentLength(size); + entry.setLastModified(lm); } entry.setLastUpdated(System.currentTimeMillis()); @@ -225,14 +236,14 @@ } /** - * Returns the 'best' valid URL for the given resource. - * This first adjusts the file name to take into account file versioning - * and packing, if possible. + * Returns the 'best' valid URL for the given resource. This first adjusts + * the file name to take into account file versioning and packing, if + * possible. * * @param resource the resource * @return the best URL, or null if all failed to resolve */ - protected URL findBestUrl(Resource resource) { + protected UrlRequestResult findBestUrl(Resource resource) { DownloadOptions options = resource.getDownloadOptions(); if (options == null) { options = new DownloadOptions(false, false); @@ -242,7 +253,6 @@ OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "Finding best URL for: " + resource.getLocation() + " : " + options.toString()); OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "All possible urls for " + resource.toString() + " : " + urls); - for (ResourceTracker.RequestMethods requestMethod : ResourceTracker.RequestMethods.getValidRequestMethods()) { for (int i = 0; i < urls.size(); i++) { URL url = urls.get(i); @@ -250,13 +260,13 @@ Map requestProperties = new HashMap<>(); requestProperties.put("Accept-Encoding", "pack200-gzip, gzip"); - CodeWithRedirect response = getUrlResponseCodeWithRedirectonResult(url, requestProperties, requestMethod); - if (response.shouldRedirect()){ + UrlRequestResult response = getUrlResponseCodeWithRedirectonResult(url, requestProperties, requestMethod); + if (response.shouldRedirect()) { if (response.URL == null) { OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "Although " + resource.toString() + " got redirect " + response.result + " code for " + requestMethod + " request for " + url.toExternalForm() + " the target was null. Not following"); } else { - OutputController.getLogger().log(OutputController.Level.MESSAGE_DEBUG, "Resource " + resource.toString() + " got redirect " + response.result + " code for " + requestMethod + " request for " + url.toExternalForm() + " adding " + response.URL.toExternalForm()+" to list of possible urls"); - if (!JNLPRuntime.isAllowRedirect()){ + OutputController.getLogger().log(OutputController.Level.MESSAGE_DEBUG, "Resource " + resource.toString() + " got redirect " + response.result + " code for " + requestMethod + " request for " + url.toExternalForm() + " adding " + response.URL.toExternalForm() + " to list of possible urls"); + if (!JNLPRuntime.isAllowRedirect()) { throw new RedirectionException("The resource " + url.toExternalForm() + " is being redirected (" + response.result + ") to " + response.URL.toExternalForm() + ". This is disabled by default. If you wont to allow it, run javaws with -allowredirect parameter."); } urls.add(response.URL); @@ -265,7 +275,11 @@ OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "For " + resource.toString() + " the server returned " + response.result + " code for " + requestMethod + " request for " + url.toExternalForm()); } else { OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "best url for " + resource.toString() + " is " + url.toString() + " by " + requestMethod); - return url; /* This is the best URL */ + if (response.URL == null) { + response.URL = url; + } + return response; /* This is the best URL */ + } } catch (IOException e) { // continue to next candidate @@ -289,18 +303,17 @@ String contentEncoding = connection.getContentEncoding(); - OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "Downloading " + downloadTo + " using " + - downloadFrom + " (encoding : " + contentEncoding + ") "); + OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG, "Downloading " + downloadTo + " using " + + downloadFrom + " (encoding : " + contentEncoding + ") "); - boolean packgz = "pack200-gzip".equals(contentEncoding) || - downloadFrom.getPath().endsWith(".pack.gz"); + boolean packgz = "pack200-gzip".equals(contentEncoding) + || downloadFrom.getPath().endsWith(".pack.gz"); boolean gzip = "gzip".equals(contentEncoding); // It's important to check packgz first. If a stream is both // pack200 and gz encoded, then con.getContentEncoding() could // return ".gz", so if we check gzip first, we would end up // treating a pack200 file as a jar file. - if (packgz) { downloadPackGzFile(resource, connection, new URL(downloadFrom + ".pack.gz"), downloadTo); } else if (gzip) { @@ -380,7 +393,7 @@ resource.incrementTransferred(rlen); out.write(buf, 0, rlen); } - + in.close(); } } @@ -393,14 +406,14 @@ try (GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(CacheUtil .getCacheFile(compressedLocation, version)))) { InputStream inputStream = new BufferedInputStream(gzInputStream); - + BufferedOutputStream outputStream = new BufferedOutputStream(new FileOutputStream(CacheUtil .getCacheFile(uncompressedLocation, version))); - + while (-1 != (rlen = inputStream.read(buf))) { outputStream.write(buf, 0, rlen); } - + outputStream.close(); inputStream.close(); } @@ -412,29 +425,51 @@ try (GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(CacheUtil .getCacheFile(compressedLocation, version)))) { InputStream inputStream = new BufferedInputStream(gzInputStream); - + JarOutputStream outputStream = new JarOutputStream(new FileOutputStream(CacheUtil .getCacheFile(uncompressedLocation, version))); - + Pack200.Unpacker unpacker = Pack200.newUnpacker(); unpacker.unpack(inputStream, outputStream); - + outputStream.close(); inputStream.close(); } } /** - * Complex wrapper around return code with utility methods - * Default is HTTP_OK + * Complex wrapper around url request Contains return code (default is + * HTTP_OK), length and last modified + * + * The storing of redirect target is quite obvious The storing length and + * last modified may be not, but appearently + * (http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=2591) the url + * conenction is not always chaced as expected, and so another request may + * be sent when length and lastmodified are checked + * */ - private static class CodeWithRedirect { + static class UrlRequestResult { int result = HttpURLConnection.HTTP_OK; URL URL; + Long lastModified; + Long length; + + public UrlRequestResult() { + } + + public UrlRequestResult(URL URL) { + this.URL = URL; + } + + URL getURL() { + return URL; + } + /** - * @return whether the result code is redirect one. Rigth now 301-303 and 307-308 + * @return whether the result code is redirect one. Rigth now 301-303 + * and 307-308 */ public boolean shouldRedirect() { return (result == 301 @@ -445,11 +480,20 @@ } /** - * @return whether the return code is OK one - anything except <200,300) + * @return whether the return code is OK one - anything except <200,300) */ public boolean isInvalid() { return (result < 200 || result >= 300); } + + @Override + public String toString() { + return "" + + "url: " + (URL == null ? "null" : URL.toExternalForm()) + "; " + + "result:" + result + "; " + + "lastModified: " + (lastModified == null ? "null" : lastModified.toString()) + "; " + + "length: " + length == null ? "null" : length.toString() + "; "; + } } private static class RedirectionException extends RuntimeException { @@ -464,5 +508,4 @@ } - } diff -r f48d4c62c2e3 -r 199702cfbca7 tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java --- a/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java Wed Jan 06 09:53:38 2016 +0100 +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java Wed Jan 06 10:49:31 2016 +0100 @@ -19,7 +19,6 @@ import java.util.jar.Pack200; import java.util.zip.GZIPOutputStream; - import org.junit.AfterClass; import org.junit.Assert; import org.junit.BeforeClass; @@ -35,7 +34,7 @@ import net.sourceforge.jnlp.util.logging.NoStdOutErrTest; import net.sourceforge.jnlp.util.logging.OutputController; -public class ResourceDownloaderTest extends NoStdOutErrTest{ +public class ResourceDownloaderTest extends NoStdOutErrTest { public static ServerLauncher testServer; public static ServerLauncher testServerWithBrokenHead; @@ -78,7 +77,6 @@ OutputController.getLogger().setOut(new PrintStream(currentErrorStream)); OutputController.getLogger().setErr(new PrintStream(currentErrorStream)); - } @AfterClass @@ -130,7 +128,8 @@ redirectErr(); try { File f = File.createTempFile(nameStub1, nameStub2); - int i = ResourceDownloader.getUrlResponseCode(testServer.getUrl(f.getName()), new HashMap(), ResourceTracker.RequestMethods.HEAD); Assert.assertEquals(HttpURLConnection.HTTP_OK, i); + int i = ResourceDownloader.getUrlResponseCode(testServer.getUrl(f.getName()), new HashMap(), ResourceTracker.RequestMethods.HEAD); + Assert.assertEquals(HttpURLConnection.HTTP_OK, i); f.delete(); i = ResourceDownloader.getUrlResponseCode(testServer.getUrl(f.getName()), new HashMap(), ResourceTracker.RequestMethods.HEAD); Assert.assertEquals(HttpURLConnection.HTTP_NOT_FOUND, i); @@ -227,30 +226,29 @@ Resource r2 = Resource.getResource(testServerWithBrokenHead.getUrl(fileForServerWithoutHeader.getName()), null, UpdatePolicy.NEVER); Resource r3 = Resource.getResource(testServer.getUrl(versionedFileForServerWithHeader.getName()), new Version("1.0"), UpdatePolicy.NEVER); Resource r4 = Resource.getResource(testServerWithBrokenHead.getUrl(versionedFileForServerWithoutHeader.getName()), new Version("1.0"), UpdatePolicy.NEVER); - assertOnServerWithHeader(resourceDownloader.findBestUrl(r1)); - assertVersionedOneOnServerWithHeader(resourceDownloader.findBestUrl(r3)); - assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2)); - assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4)); + assertOnServerWithHeader(resourceDownloader.findBestUrl(r1).getURL()); + assertVersionedOneOnServerWithHeader(resourceDownloader.findBestUrl(r3).URL); + assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2).URL); + assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4).URL); fileForServerWithHeader.delete(); Assert.assertNull(resourceDownloader.findBestUrl(r1)); - assertVersionedOneOnServerWithHeader(resourceDownloader.findBestUrl(r3)); - assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2)); - assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4)); + assertVersionedOneOnServerWithHeader(resourceDownloader.findBestUrl(r3).URL); + assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2).URL); + assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4).URL); versionedFileForServerWithHeader.delete(); Assert.assertNull(resourceDownloader.findBestUrl(r1)); Assert.assertNull(resourceDownloader.findBestUrl(r3)); - assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2)); - assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4)); + assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2).URL); + assertVersionedOneOnServerWithoutHeader(resourceDownloader.findBestUrl(r4).URL); versionedFileForServerWithoutHeader.delete(); Assert.assertNull(resourceDownloader.findBestUrl(r1)); Assert.assertNull(resourceDownloader.findBestUrl(r3)); - assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2)); + assertOnServerWithoutHeader(resourceDownloader.findBestUrl(r2).URL); Assert.assertNull(resourceDownloader.findBestUrl(r4)); - fileForServerWithoutHeader.delete(); Assert.assertNull(resourceDownloader.findBestUrl(r1)); Assert.assertNull(resourceDownloader.findBestUrl(r3)); @@ -283,6 +281,7 @@ assertPort(u, testServer.getPort()); assertVersion(u); } + private void assertCommonComponentsOfUrl(URL u) { Assert.assertTrue(u.getProtocol().equals("http")); Assert.assertTrue(u.getHost().equals("localhost")); @@ -311,7 +310,7 @@ redirectErrBack(); cacheDir = PathsAndFiles.CACHE_DIR.getFullPath(); - PathsAndFiles.CACHE_DIR.setValue(System.getProperty("java.io.tmpdir") + File.separator + "tempcache"); + PathsAndFiles.CACHE_DIR.setValue(System.getProperty("java.io.tmpdir") + File.separator + "tempcache"); } @AfterClass