Closed ettore closed 2 months ago
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:
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.
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 let
s but that's a much bigger refactor.
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
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.
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?
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 the
delegate`. π I did not notice any problems with this. π
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 https://github.com/readium/swift-toolkit/pull/400 look great to me.
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:
EPUBSpreadView
is presented but the error received from theGCDHTTPServer::handlers
is lost.Readium version
2.6.1
OS version
iOS 17
Testing device
iPhone 11 and various simulators
Environment
Additional context
This was initially discussed in the Readium Slack, with the following suggestion:
NavigatorDelegate.navigator(_: Navigator, didFailToLoadResourceAt href: String, withError error: ResourceError)
;GCDHTTPServer
to the navigator delegate, for example by adding aonFailure: (_ href: String, _ error: ResourceError) -> Void
parameter to all theserve...()
variants in the HTTPServer protocol.