Errors while loading a Resource in GCDHTTPServer are not bubbled up about swift-toolkit HOT 5 CLOSED

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.

from swift-toolkit.

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

from swift-toolkit.

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.

from swift-toolkit.

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?

from swift-toolkit.

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. 😅

from swift-toolkit.