Errors while loading a Resource in GCDHTTPServer are not bubbled up

[ettore]

Describe the bug

If GCDWebServer fails to load a resource, the GCDHTTPServer adapter correctly catches the error and responds with a GCDWebServerErrorResponse (Bad Request 400). However in doing so it loses the contextual information of the error object, because the latter is not part of the response. This can be observed clearly in the implementation of the GCDHTTPServer::handle(request:) function, where the only thing that is transmitted up is the HTTP status code of the error. The error object would be useful to the app that uses Readium to display an informative error message to the user.

How to reproduce?

The following is just an example easily reproducible by using a DRM system conforming to the ContentProtection protocol:

1. Manually corrupt a DRM key or other asset that will prevent the decryption of any EPUB resource.
2. Put a breakpoint in the failure case of the GCDHTTPServer::handle(request:) function.
3. Attempt to open the EPUB.
4. Observe how EPUBSpreadView is presented but the error received from the GCDHTTPServer::handlers is lost.

Readium version

2.6.1

OS version

iOS 17

Testing device

iPhone 11 and various simulators

Environment

macOS: 13.5
platform: arm64
carthage: 0.39.1
Build version 15A507

Additional context

This was initially discussed in the Readium Slack, with the following suggestion:

• expand the NavigatorDelegate API with something like NavigatorDelegate.navigator(_: Navigator, didFailToLoadResourceAt href: String, withError error: ResourceError);
• forward the error from GCDHTTPServer to the navigator delegate, for example by adding a onFailure: (_ href: String, _ error: ResourceError) -> Void parameter to all the serve...() variants in the HTTPServer protocol.

[ettore]

I have a WIP branch on my fork, where I have 3 commits implementing 3 strategies to address the kind of problems described in this issue. Two of them address more generic use cases, while the most recent commit is what actually fixes my problem. I will describe them from last to first chronologically:

1. the most recent commit uses a change in GCDWebServer where I'm saving the error object in the GCDWebErrorResponse. This is nice to have because the response object should have all the available error details, not just the HTTP status code. However, I have not been able to actually use the error from the response because I'm getting lost in how the response is passed back.

2. the 2nd commit implements your suggestion of trying to forward the error from the serve(...) HTTPServer methods via a onFailure param. Unfortunately the errors I am seeing do not hit any of those methods. I am also not particularly happy about the way I had to expand the signatures of the various NavigatorViewControllers. What I really wanted to do (for example in EPUBNavigatorViewController) is something like

    let viewModel = try EPUBNavigatorViewModel(
        publication: publication,
        config: config,
        httpServer: httpServer,
        httpFailureHandler: { href, url, error in
             self.delegate?.navigator(self, didFailToLoadResourceAt: href ?? "", url: url, withError: error) //<--compile error
        })

    self.init(
        viewModel: viewModel,
        ....)

however that doesn't compile because I'm using self before init. So I kept delegating up the initialization of the onFailure handler because the serve(...) methods are mostly called from the initializers. A possible solution would be to make some variables such as the viewModel no longer lets but that's a much bigger refactor.

3. Finally the oldest commit solves my problem by simply providing a failure handler in GCDHTTPServer. The reasoning here originates from the way GCDHTTPServer works, which is by setting up handlers that are called asynchronously from the serve() methods. So in a way the handle(request:completion:) method is sort of a continuation of the serve(at:handler:) method; however there's not much to do in the actual serve(...) method because all we do there is assign the handler to the dictionary of handlers. Another solution I thought of would be to save the onFailure closure together with the handler param, so something like

    handlers[endpoint] = (handler: handler, onFailure: onFailure)

But that also seems kinda wrong (handler can already return a FailureResource) and most importantly I still have the problem of point (2) where I can't initialize onFailure to call the navigator delegate from within the EPUBNavigatorViewModel::init().

These 3 commits are fairly independent so please let me know what of these strategies make sense to you and what doesn't. I can then rearrange the code accordingly.

EDIT (16/3/24): minor improvements, updated branch and commits

[mickael-menu]

Unfortunately the errors I am seeing do not hit any of those methods.

Do you have any idea why that is the case?

I am also not particularly happy about the way I had to expand the signatures of the various NavigatorViewControllers.

Yes I fear we're leaking implementation details if the app has to explicitly supply an httpFailureHandler. That might be an issue if we get rid of the HTTP server on some navigators. Ideally, we should inform about any resource-specific errors using a dedicated callback from the Navigator delegate.

however that doesn't compile because I'm using self before init.

When I face this kind of issue I usually use a force-unwrapped type and initialize after self.init(). Let me know if that helps.

private(set) var property: Type!

init() {
    super.init()

    property = Type(self)
}

For the third solution, unfortunately the HTTPServer is likely a singleton in the app, so setting the failureHandle in a given navigator might break other clients of the server. E.g. if you have two navigators side by side. Maybe the solution with pairing an endpoint to the error handler is worth exploring.

[ettore]

Unfortunately the errors I am seeing do not hit any of those methods.

Do you have any idea why that is the case?

It's because the requests for serving actual EPUB resources all end up in the GCDHTTPServer::serve(at:handler:) method that adds to the handlers array.

I am also not particularly happy about the way I had to expand the signatures of the various NavigatorViewControllers.

Yes I fear we're leaking implementation details if the app has to explicitly supply an httpFailureHandler.

The app doesn't anymore, but the EPUBNavigatorViewController does.

do you mean we'd be leaking implementation details of the GCDHTTPServer to the navigators? I sort of assumed that since GCDHTTPServer is in the same repo and conceptually it's just an adapter, we'd be ok to have this kind of hook. However the one problem I see with the httpFailureHandler solution is that it's not thread safe. That's definitely an issue, and I didn't consider that that its possible to have 2 Navigators side by side with the same server.

When I face this kind of issue I usually use a force-unwrapped type and initialize after self.init(). Let me know if that helps.

Implicitly unwrapped optionals do make me nervous but in this case it seems ok. 👍 My go-to is often just making the init dumber and delegate the actual startup to later, but in this case it would be a giant refractor.

Maybe the solution with pairing an endpoint to the error handler is worth exploring.

Do you mean this kind of pairing:

handlers[endpoint] = (handler: handler, onFailure: onFailure)

I'll try that in conjunction with implicit unwraps, and open a PR.

I still think the best and simplest solution would be to find a way to use the GCDWebServerErrorResponse. I have a GCDHTTPServer PR for adding the NSError that happened to it. However I can't understand how and where I can then use the GCDWebServerErrorResponse from. It seems to reach GCDWebServerConnection but how can I observe it from the client?

[ettore]

alright! I like this solution a lot more. Client-facing Navigator API did not change.

The notable change is that the publicationBaseURL in the EPUBViewModel class is now implicitly unwrapped and it is initialized after the call to the designated init, but it IS initialized right after, and still inside the convenience initializer. This allowed me to also invoke httpServer.serve(...) right after the designated init, and therefore set the failureHandlerto call thedelegate`. 🎉 I did not notice any problems with this. 😅

[mickael-menu]

do you mean we'd be leaking implementation details of the GCDHTTPServer to the navigators? I sort of assumed that since GCDHTTPServer is in the same repo and conceptually it's just an adapter, we'd be ok to have this kind of hook. However the one problem I see with the httpFailureHandler solution is that it's not thread safe. That's definitely an issue, and I didn't consider that that its possible to have 2 Navigators side by side with the same server.

We can have the EPUBNavigatorViewController depend on APIs from HTTPServer but not GCDHTTPServer directly.
I meant that host apps shouldn't not need to pass a httpFailureHandler to the initializer of EPUBNavigatorViewController, instead of handling the error in a NavigatorDelegate callback.

But the changes you made in #400 look great to me.