I think that the scope for fossil.fetch.onerror() is too broad. The only recoverable error conditions are probably the ones handed to XMLHttpRequest.onerror(), i.e. temporary network connectivity problems, or temporary 504 Gateway Timeout server outages. Anything else, including malformed JSON replies, is unlikely to be recoverable, and the default way of script errors appearing in the developer console seems appropriate.

onerror() has never been intended for recovery, only for error reporting, and it's used that way extensively in the other JS-based apps. At that level generic API there is no sensible recovery which can he attempted and the error reporting is necessarily app-dependent (/chat does it differently than /wikiedit or /pikchrshow do). The recovery strategy for all of those apps is "alert the human and let them figure out what to do." Having errors only appear in the dev console makes them useless on mobile platforms (like this tablet) and makes tracing down problems on those platforms difficult for the developers.

Having errors only appear in the dev console makes them useless on mobile platforms ...

Oh my! Bless all the poor souls, thinking they never get any matches, not realizing their online dating mobile app crashed due to a Javascript error ... :(

So let's hope the bugs taking the more likely path with the msg widget still around. Or, even better, let's hope there's no bugs at all! Or, maybe I should just stop looking for new ones every other day. ;-)

Or, maybe I should just stop looking for new ones every other day. ;-)

...

The main consequence of the above is that the [current error handler][1] is buggy, as it assumes the wrapping Diff object and its msg widget and fetch queue are still alive.

Please recall that i warned, at least three times, that the fetch queue added error cases which we didn't previously have ;).

Please recall that i warned, at least three times, that the fetch queue added error cases which we didn't previously have ;).

The problem is not related to fetch queue management.

Any access to the queue is guarded by checks whether it still exists (or if the wrapping object was destroyed). This seems robust to me, as Javascript doesn't run in some multi-threaded environment requiring more sophisticated synchronization.

The initial code I added to the pre-existing error handler just reset the queue to an empty array, because I found this was safest, as it would work in all situations, even with the wrapping object destroyed.

But then during further development I noticed the odd behavior of the fossil.fetch error handler, i.e. that it can be fired long after successful completion of the fetch request.

Because this was unexpected and undocumented, and because I noticed that your code in the error handler also ignored this (by assuming the msg widget is still alive, which of course may be wrong in case of an error you did not expect when writing the code, or due to some condition on platforms you didn't test), I thought that the overly broad scope of the error handler was a bug, and that even you as the original author "forgot" about this, and "got it wrong". So I decided to do the same (by check-in [36ba7ec968]), expecting there would soon be a fix for the error handler.

But I'm okay with your explanation that the purpose of the error handler is to work around the limitations of mobile platforms. Still, it seems a bit strange to have an error handler of a class named "fetch" and use it as a general-purpose error handler.

But then during further development I noticed the odd behavior of the fossil.fetch error handler, i.e. that it can be fired long after successful completion of the fetch request.

i will add documentation explaining that and change the onerror handler to:

      fOpt.onerror = function(err){
        if(self.e/*guard against a late-stage onerror() call*/){
          self.msg(true,err.message);
          self.$fetchQueue.length = 0;
        }else{
          Diff.config.chunkFetch.onerror.call(this,e);
        }
      };

That will ensure that any "very late" onerror() call simply goes to the console. (We don't care if users don't see that particular message because the relevant DOM elements have already been removed from the page at that point.)

But I'm okay with your explanation that the purpose of the error handler is to work around the limitations of mobile platforms. Still, it seems a bit strange to have an error handler of a class named "fetch" and use it as a general-purpose error handler.

It has never been intended to do anything more than report problems to the user (whether mobile or not). My argument regarding mobile is only that console.error() is not a solution for that platform. If users experience silent errors, they come to us and say, "it's failing silently!" in which case we cannot really help them (and it's our fault!). That applies to every platform, but especially on mobile because only users who hook up a remote debugger can see those messages.

Thanks for the fixes!

Or ... should this line:

Diff.config.chunkFetch.onerror.call(this,e);

Be changed to:

Diff.config.chunkFetch.onerror.call(this,err);

Diff.config.chunkFetch.onerror.call(this,err);

Of course it should! Fixed and thank you for catching that.