Improve tile error handling #3104
Conversation
|
Can confirm WMTS problem is fixed. |
|
Can confirm unrelated CSV problem fixed. |
|
Can confirm that we are now very much more resistant to 503's at least, tested on AREMI with the following layers plus Australian Topography basemap, and it took a very large number of 503's before the layers started to actually fail. These show the ones that did and did not fail + Australian Topography:
I probably got 1000 or so 503's before things started failing, so I'd say we should go ahead and merge. |
| @@ -451,8 +451,10 @@ function calculateFinishDatesFromStartDates(startJulianDates, localDefaultFinalD | |||
| var n = sortedUniqueJulianDates.length; | |||
| if (n > 1) { | |||
| finalDurationSeconds = JulianDate.secondsDifference(sortedUniqueJulianDates[n - 1], sortedUniqueJulianDates[0]) / (n - 1); | |||
| endDates.push(JulianDate.addSeconds(sortedUniqueJulianDates[n - 1], finalDurationSeconds, new JulianDate())); | |||
| } else { | |||
| endDates.push(undefined); | |||
There was a problem hiding this comment.
@kring Is this the right thing to do for n === 1?
There was a problem hiding this comment.
😬 I think so. The way endDates is used below, it looks like there has to be something added, or the indices will be wrong.
| tileFailures = 0; | ||
| tileRetriesByMap = {}; |
There was a problem hiding this comment.
retriesPerTile would be a better name.
There was a problem hiding this comment.
Also, is it definitely correct to reset it whenever there is any "intervening success"? If tile X has failed 3 times, then some other random tile succeeds, will this (incorrectly) get reset?
There was a problem hiding this comment.
It's definitely not correct, but the effect should be that TerriaJS is more forgiving of errors, which is closer to a :feature: than a 🐛.
| }); | ||
| } else if (!defined(e.statusCode)) { | ||
| // On a modern-ish browser, this is almost certainly a CORS problem. | ||
| // Note that ignoreUnknownTileErrors is only for genuinely unknown (no status code) errors. |
There was a problem hiding this comment.
This situation also arises on DNS failures (eg, non existent domain)
|
@stevage reports that https://tiles.openaerialmap.org/5abae68e65bd8f00110f3e42/0/5abae68e65bd8f00110f3e43/wmts doesn't work on http://ci.terria.io/fail-better/ |
|
Actually the last time I looked at the CI, it seemed to be fetching tiles, but then failing silently. I really don't know what's going on. Possibly totally spurious. |
|
Ok here's what's happening with that openaerialmap server... The extent of the one layer on that server is a tiny little square near Cape Code, MA, USA. So when you're looking at Australia, Cesium only downloads a couple of tiles. But those tiles all return XML, probably because of #2927, which mentions this exact server. Because we only download a couple of tiles, though, it doesn't fail and show the user a message immediately. But if you try to zoom to the layer's extent, you'll get an error like this pretty quickly:
So I think everything is working as expected, but let me know if anything still seems off to you @stevage. |
|
I think I've addressed all the review comments @steve9164 and @stevage. |
More often than not, it will be the browser denying access on the basis that the server didn't provide any CORS instructions at all, right? |
* stevage/fail-better: Comments and other minor improvements. # Conflicts: # lib/Models/ImageryLayerCatalogItem.js
|
(I thought I had already pushed these commits, but I pushed them to the wrong branch...) |
Very true, but I think explaining that just muddies the water. The browser can't be changed, but the server potentially can be, so it's reasonable to explain it as something that has to change on the server. From the browser's perspective, a lack of explicit approval is the same as denial. |
Fixes #2928 (both the WMTS problem and the unrelated CSV problem)
Closes #3083
Is probably a reasonable workaround for #3092
Sadly I didn't really make this code any more sane. It's still totally crazy.