Changeset 8433 in josm

Timestamp:

2015-05-28T22:00:41+02:00 (9 years ago)

Author:

wiktorn

Message:

Clean up handling of various error situations. Closes #11494

Location:

trunk/src/org/openstreetmap/josm/data

Files:

• cache/JCSCachedTileLoaderJob.java (modified) (8 diffs)
• imagery/TMSCachedTileLoaderJob.java (modified) (5 diffs)

trunk/src/org/openstreetmap/josm/data/cache/JCSCachedTileLoaderJob.java

@@ -85 +85 @@
     public static ThreadFactory getNamedThreadFactory(final String name) {
         return new ThreadFactory(){
+            @Override
             public Thread newThread(Runnable r) {
                 Thread t = Executors.defaultThreadFactory().newThread(r);
@@ -185 +186 @@
         if (first || force) {
             ensureCacheElement();
-            if (!force && cacheElement != null && isCacheElementValid() && (isObjectLoadable())) {
+            if (!force && cacheElement != null && isCacheElementValid() && isObjectLoadable()) {
                 // we got something in cache, and it's valid, so lets return it
                 log.log(Level.FINE, "JCS - Returning object from cache: {0}", getCacheKey());
@@ -215 +216 @@
 
     /**
+     * Simple implementation. All errors should be cached as empty. Though some JDK (JDK8 on Windows for example)
+     * doesn't return 4xx error codes, instead they do throw an FileNotFoundException or IOException
      *
-     * @return cache object as empty, regardless of what remote resource has returned (ex. based on headers)
-     */
-    protected boolean cacheAsEmpty(Map<String, List<String>> headers, int statusCode, byte[] content) {
-        return false;
+     * @return true if we should put empty object into cache, regardless of what remote resource has returned
+     */
+    protected boolean cacheAsEmpty() {
+        return attributes.getResponseCode() < 500;
     }
 
@@ -262 +265 @@
     }
 
-
     private void finishLoading(LoadResult result) {
         Set<ICachedLoaderListener> listeners = null;
@@ -272 +274 @@
             return;
         }
-        try {
-            for (ICachedLoaderListener l: listeners) {
-                l.loadingFinished(cacheData, attributes, result);
-            }
-        } catch (Exception e) {
-            log.log(Level.WARNING, "JCS - Error while loading object from cache: {0}; {1}", new Object[]{e.getMessage(), getUrl()});
-            Main.warn(e);
-            for (ICachedLoaderListener l: listeners) {
-                l.loadingFinished(cacheData, attributes, LoadResult.FAILURE);
-            }
-
-        }
-
+        for (ICachedLoaderListener l: listeners) {
+            l.loadingFinished(cacheData, attributes, result);
+        }
     }
 
@@ -362 +354 @@
                 byte[] raw = read(urlConn);
 
-                if (!cacheAsEmpty(urlConn.getHeaderFields(), urlConn.getResponseCode(), raw) &&
-                        raw != null && raw.length > 0) {
+                if (isResponseLoadable(urlConn.getHeaderFields(), urlConn.getResponseCode(), raw)) {
                     // we need to check cacheEmpty, so for cases, when data is returned, but we want to store
                     // as empty (eg. empty tile images) to save some space
@@ -371 +362 @@
                             new Object[] {getCacheKey(), raw.length, getUrl()});
                     return true;
-                } else  {
+                } else if (cacheAsEmpty()) {
                     cacheData = createCacheEntry(new byte[]{});
                     cache.put(getCacheKey(), cacheData, attributes);
                     log.log(Level.FINE, "JCS - Caching empty object {0}", getUrl());
                     return true;
+                } else {
+                    log.log(Level.FINE, "JCS - failure during load - reponse is not loadable nor cached as empty");
+                    return false;
                 }
             }
         } catch (FileNotFoundException e) {
             log.log(Level.FINE, "JCS - Caching empty object as server returned 404 for: {0}", getUrl());
-            cache.put(getCacheKey(), createCacheEntry(new byte[]{}), attributes);
-            return handleNotFound();
+            attributes.setResponseCode(404);
+            boolean doCache = isResponseLoadable(null, 404, null) || cacheAsEmpty();
+            if (doCache) {
+                cacheData = createCacheEntry(new byte[]{});
+                cache.put(getCacheKey(), cacheData, attributes);
+            }
+            return doCache;
+        } catch (IOException e) {
+            log.log(Level.FINE, "JCS - IOExecption during communication with server for: {0}", getUrl());
+            attributes.setResponseCode(499); // set dummy error code
+            boolean doCache = isResponseLoadable(null, 499, null) || cacheAsEmpty(); //generic 499 error code returned
+            if (doCache) {
+                cacheData = createCacheEntry(new byte[]{});
+                cache.put(getCacheKey(), createCacheEntry(new byte[]{}), attributes);
+            }
+            return doCache;
         } catch (Exception e) {
             log.log(Level.WARNING, "JCS - Exception during download {0}",  getUrl());
@@ -392 +400 @@
 
     /**
-     *  @return if we should treat this object as properly loaded
-     */
-    protected abstract boolean handleNotFound();
+     * Check if the object is loadable. This means, if the data will be parsed, and if this response
+     * will finish as successful retrieve.
+     *
+     * This simple implementation doesn't load empty response, nor client (4xx) and server (5xx) errors
+     *
+     * @param headerFields headers sent by server
+     * @param responseCode http status code
+     * @param raw data read from server
+     * @return true if object should be cached and returned to listener
+     */
+    protected boolean isResponseLoadable(Map<String, List<String>> headerFields, int responseCode, byte[] raw) {
+        if (raw == null || raw.length == 0 || responseCode >= 400) {
+            return false;
+        }
+        return true;
+    }
 
     protected abstract V createCacheEntry(byte[] content);

trunk/src/org/openstreetmap/josm/data/imagery/TMSCachedTileLoaderJob.java

@@ -137 +137 @@
 
     @Override
-    protected boolean cacheAsEmpty(Map<String, List<String>> headers, int statusCode, byte[] content) {
-        // cacheAsEmpty is called for every successful download, so we can put
-        // metadata handling here
+    protected boolean isResponseLoadable(Map<String, List<String>> headers, int statusCode, byte[] content) {
         attributes.setMetadata(tile.getTileSource().getMetadata(headers));
         if (tile.getTileSource().isNoTileAtZoom(headers, statusCode, content)) {
             attributes.setNoTileAtZoom(true);
-            return true;
-        }
-        return false;
+            return false; // do no try to load data from no-tile at zoom, cache empty object instead
+        }
+        return super.isResponseLoadable(headers, statusCode, content);
+    }
+
+    @Override
+    protected boolean cacheAsEmpty() {
+        return isNoTileAtZoom() || super.cacheAsEmpty();
     }
 
@@ -171 +174 @@
             listeners = inProgress.remove(getCacheKey());
         }
+        boolean status = result.equals(LoadResult.SUCCESS);
 
         try {
@@ -194 +198 @@
                     if (!isNoTileAtZoom() && httpStatusCode >= 400) {
                         tile.setError(tr("HTTP error {0} when loading tiles", httpStatusCode));
+                        status = false;
                     }
                     break;
@@ -207 +212 @@
             if (listeners != null) { // listeners might be null, if some other thread notified already about success
                 for(TileLoaderListener l: listeners) {
-                    l.tileLoadingFinished(tile, result.equals(LoadResult.SUCCESS));
+                    l.tileLoadingFinished(tile, status);
                 }
             }
@@ -259 +264 @@
     }
 
-    @Override
-    protected boolean handleNotFound() {
-        if (tile.getSource().isNoTileAtZoom(null, 404, null)) {
-            tile.setError("No tile at this zoom level");
-            tile.putValue("tile-info", "no-tile");
-            return true;
-        }
-        return false;
-    }
-
     /**
      * For TMS use BaseURL as settings discovery, so for different paths, we will have different settings (useful for developer servers)