Don't use Axios interceptor for error handling

[qstokkink]

In #8120, I introduced an error reporter that uses an Axios interceptor. The intent was that this would only intercept unhandled errors.

What actually happens is that this interceptor is called BEFORE any handlers. This means that every single non-2xx HTTP code is reported as an error, regardless of whether it is handled later or not.

Regrettably, the implication here is that we have to attach the error handler manually to every single request in the Tribler + IPv8 services. For example

async getDownloadFiles(infohash: string): Promise<BTFile[]> {
    return (await this.http.get(`/downloads/${infohash}/files`)).data.files;
}

.. turns into something like ..

async getDownloadFiles(infohash: string): Promise<BTFile[]> {
    try {
        return (await this.http.get(`/downloads/${infohash}/files`)).data.files;
    } catch (error) {
        if (axios.isAxiosError(error) && error.response?.status == "404") {
            return new Array();
        } else {
            handleHTTPError(error);
        }
    }
}

This will roughly quadruple the file sizes of the services. It would be nice if there was some way to get this shorter.

[qstokkink]

This is still not short, but I think this is already an improvement:

async getDownloadFiles(infohash: string): Promise<BTFile[]> {
    const response = (await this.http.get(`/downloads/${infohash}/files`)).catch(handleHTTPError);
    switch (response.status){
      case '200':
        return response.data.files;
      case '404':
        return new Array();
      default:
        // throw an error?
    }
}

EDIT: This still may not be doing what I want it to do.

[qstokkink]

Hold on, according to the docs you can specify per status code whether it should count as an error:

const response = this.http.get(`/downloads/${infohash}/files`,
                                 { validateStatus: function (status) { return [200, 404].includes(status); } }
                              ).catch(handleHTTPError);
switch (response.status){
  case '404':
    return new Array();
  default:
    return response.data.files;
}

I guess we can create a vararg helper method handles(...) to shorten this further to

const response = this.http.get(`/downloads/${infohash}/files`, handles(200, 404)).catch(handleHTTPError);

[qstokkink]

@egbertbouman This will be a very invasive change. Are you OK with the proposal of my previous/next post? Or, do you know of other options?

[qstokkink]

Here's a real patch (in this case the code actually becomes shorter):

diff --git a/src/tribler/ui/src/services/reporting.ts b/src/tribler/ui/src/services/reporting.ts
index 3bd58b6fe..35be66805 100644
--- a/src/tribler/ui/src/services/reporting.ts
+++ b/src/tribler/ui/src/services/reporting.ts
@@ -17,3 +17,7 @@ export function handleHTTPError(error: Error | AxiosError) {
     }
     return Promise.reject(error);
 }
+
+export function handles(...handled: number[]) {
+    return { validateStatus: function (status) { return handled.includes(status); } }
+}
diff --git a/src/tribler/ui/src/services/tribler.service.ts b/src/tribler/ui/src/services/tribler.service.ts
index d5d6ce36e..bd221b025 100644
--- a/src/tribler/ui/src/services/tribler.service.ts
+++ b/src/tribler/ui/src/services/tribler.service.ts
@@ -5,7 +5,7 @@ import { Path } from "@/models/path.model";
 import { GuiSettings, Settings } from "@/models/settings.model";
 import { Torrent } from "@/models/torrent.model";
 import axios, { AxiosError, AxiosInstance } from "axios";
-import { handleHTTPError } from "./reporting";
+import { handleHTTPError, handles } from "./reporting";

 const OnError = (event: MessageEvent) => {
@@ -26,7 +26,6 @@ export class TriblerService {
             baseURL: this.baseURL,
             withCredentials: true,
         });
-        this.http.interceptors.response.use(function (response) { return response; }, handleHTTPError);
         this.events = new EventSource(this.baseURL + '/events', { withCredentials: true });
         this.addEventListener("tribler_exception", OnError);
         // Gets the GuiSettings
@@ -118,14 +117,7 @@ export class TriblerService {
     // Torrents / search

     async getMetainfo(uri: string) {
-        try {
-            return (await this.http.get(`/torrentinfo?uri=${uri}`)).data;
-        }
-        catch (error) {
-            if (axios.isAxiosError(error)) {
-                return error.response?.data;
-            }
-        }
+        return (await this.http.get(`/torrentinfo?uri=${uri}`, handles(200, 400, 500)).catch(handleHTTPError)).data;
     }

     async getMetainfoFromFile(torrent: File) {

[qstokkink]

Unless we find something better, I guess this is the most workable solution. I'll start implementing this. It'll take a little while.

[qstokkink]

I dropped off the proof of concept in the PRs. Let's leave it open for a bit and see if we can think of something nicer.

[qstokkink]

Important lesson learned from the POC: we HAVE TO handle ALL HTTP status codes that may occur when calling either the triblerservice or the ipv8service.

What was happening in the POC is this:

1. An unexpected error occurs in the core and it sends an HTTP 500 to the GUI.
2. The GUI notices an unexpected status code and shows the error reporter.
3. The failing call returns and the oblivious caller attempts to process the response.
4. The failing call does not have a normal response and crashes the oblivious caller. The GUI crashes.
5. Because the GUI crashed, the error reporter is "invisible"/gone.

Essentially, I tried to be lazy and just show the reporter and let the call crash, but letting the call crash means that the entire GUI disappears.

[qstokkink]

Ok, back to the drawing board. In order to get a vision for the next POC, I'll formulate the problem more concretely now. Let's see if we can work off of the pattern of getMetainfo.

https://github.com/Tribler/tribler/blob/56b822c3a9b56dc6bf1ebfa1151bcb35a90c28d1/src/tribler/ui/src/dialogs/SaveAs.tsx#L108-L122

https://github.com/Tribler/tribler/blob/c2e09c6d65f6ed15e692035f7864e5cbbe239574/src/tribler/ui/src/services/tribler.service.ts#L120-L129

The above is not yet complete. There are three different types of errors that should be handled, and one error that we should crash on:

1. An AxiosError with an expected non-200 HTTP status. This CAN also be a 500 status (see next point).
2. An AxiosError with an unexpected 500 HTTP status.
3. An AxiosError with a raw network error.

4. We could also get a base Error but that should not be fixed in the handler - so I suggest just throwing that again and crashing.

   The current implementation of getMetainfo implements points 1 and 4. So, the concrete problem is this:

   • We somehow need to support points 2 and 3.
   • I'd prefer an elegant solution because there are a lot of functions in the services (and whomever implements this will have to repeat this pattern many times).
   • If possible, the service interface should not allow you to forget about points 1, 2, and 3.

[qstokkink]

Documenting more off-GitHub discussions.

As pointed out by @egbertbouman, error responses that are not handled have an additional key in the response dictionary: handled: false. With this info we can distinguish points 1 and 2.

Regarding point 3, there are 12 non-user errors that may occur, see: https://github.com/axios/axios?tab=readme-ov-file#error-types Explicitly handling these would require a construction like the following:

if (error.code === 'ERR_NETWORK') { ... 
} else if (error.code === 'ERR_CANCELED') { ... 
} ... etc

However, the documentation also suggests that simply checking if (error.response) is sufficient to simply group up all of the point-3 type errors. For our purposes that should be sufficient.

With that, I think we have all the errors covered that can occur inside of the service handlers. It would look something (untested) like the following:

 async getMetainfo(uri: string) { 
     try { 
         return (await this.http.get(`/torrentinfo?uri=${uri}`)).data; 
     } 
     catch (error) { 
         if (axios.isAxiosError(error)) { 
             if (error.response) return "RAW_AXIOS_ERROR";
             if (error.response?.data?.error?.handled && error.response.data.error.handled == false) handleHTTPError(error);
             return error.response?.data;
         } 
         throw error;
     } 
 } 

Now, there is still the matter of creating a response for the caller of the service. Currently, every caller of such a service function would require three checks (edit: in only two blocks):

1. Is the error a raw AxiosError or an unhandeld error? If so, unelegantly recover.
2. Is the error an expected error? If so, continue the unhappy flow.
3. Otherwise, continue with the happy flow.

[qstokkink]

Perhaps it makes more sense to NOT return the error dictionary to the caller of a service function when handled == false. The callers can probably not do anything useful with this info anyway and this allows them to lump in the "raw AxiosError or an unhandeld error" with a single check.

Given all of the above, I'm thinking the following might be a convenient signature:

 service.method(): Promise< HAPPY_FLOW_TYPE | undefined > { ... }

This allows the typing system to detect callers of this function that do not handle undefined. So, this would ensure that unexpected errors are handled.

[qstokkink]

Memo (for myself): there is yet another type of error that could come from the core:

return RESTResponse(body=b"This is not JSON, oh no!")

[qstokkink]

Alright, the failsafe has been merged in #8150

To properly close this issue, we still need a PR that looks at all the REST responses and gracefully handles them in the GUI.