Proof of concept to replace the HTTP server with WKURLSchemeHandler #160
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| // | ||
| // Copyright 2021 Readium Foundation. All rights reserved. | ||
| // Use of this source code is governed by the BSD-style license | ||
| // available in the top-level LICENSE file of the project. | ||
| // | ||
|
|
||
| import Foundation | ||
| import R2Shared | ||
| import WebKit | ||
|
|
||
| @available(iOS 11.0, *) | ||
| final class WebViewResourceHandler: NSObject, WKURLSchemeHandler, Loggable { | ||
|
|
||
| enum HandlerError: Error { | ||
| case noURLProvided | ||
| case unsupportedScheme(String?) | ||
| case taskNotStarted(WKURLSchemeTask) | ||
| } | ||
|
|
||
| private let scheme: String | ||
| private let publication: Publication | ||
| private var tasks: [ObjectIdentifier: Task] = [:] | ||
|
|
||
| init(scheme: String, publication: Publication) { | ||
| self.scheme = scheme | ||
| self.publication = publication | ||
| } | ||
|
|
||
| func webView(_ webView: WKWebView, start urlSchemeTask: WKURLSchemeTask) { | ||
| guard let url = urlSchemeTask.request.url else { | ||
| urlSchemeTask.didFailWithError(HandlerError.noURLProvided) | ||
| return | ||
| } | ||
| guard url.scheme == scheme else { | ||
| urlSchemeTask.didFailWithError(HandlerError.unsupportedScheme(url.scheme)) | ||
| return | ||
| } | ||
|
|
||
| let href = url.absoluteString.removingPrefix(scheme + "://") | ||
| let resource = publication.get(href) | ||
| let task = Task(url: url, task: urlSchemeTask, resource: resource) | ||
| tasks[ObjectIdentifier(urlSchemeTask)] = task | ||
mickael-menu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| task.start() | ||
| } | ||
|
|
||
| func webView(_ webView: WKWebView, stop urlSchemeTask: WKURLSchemeTask) { | ||
| guard let task = tasks[ObjectIdentifier(urlSchemeTask)] else { | ||
| urlSchemeTask.didFailWithError(HandlerError.taskNotStarted(urlSchemeTask)) | ||
| return | ||
| } | ||
|
|
||
| task.cancel() | ||
| } | ||
|
|
||
| private final class Task { | ||
| private let url: URL | ||
| private let resource: Resource | ||
| private var isCancelled = false | ||
|
|
||
| /// Underlying WKURLSchemeTask. | ||
| /// Use `withTask()` to access it safely. | ||
| private let _task: WKURLSchemeTask | ||
|
|
||
| init(url: URL, task: WKURLSchemeTask, resource: Resource) { | ||
| self.url = url | ||
| self.resource = resource | ||
| self._task = task | ||
| } | ||
|
|
||
| func start() { | ||
| let request = _task.request | ||
|
|
||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| let href = self.resource.link.href | ||
| do { | ||
| log(.info, "Will serve \(href), headers: \(request.allHTTPHeaderFields ?? [:])") | ||
| let length = try self.resource.length.get() | ||
| let response = URLResponse( | ||
| url: self.url, | ||
| mimeType: self.resource.link.type, | ||
| expectedContentLength: Int(length), | ||
| textEncodingName: nil | ||
| ) | ||
| self.withTask { $0.didReceive(response) } | ||
|
|
||
| var available = length | ||
| var offset: UInt64 = 0 | ||
| var data: Data = Data() | ||
| repeat { | ||
| let upperBound = offset + min(available, 32 * 1024) | ||
| data = try self.resource.read(range: offset..<upperBound).get() | ||
|
|
||
| offset += UInt64(data.count) | ||
| available -= UInt64(data.count) | ||
|
|
||
| self.withTask { $0.didReceive(data) } | ||
| } while available > 0 && !self.isCancelled | ||
|
|
||
| self.withTask { $0.didFinish() } | ||
|
|
||
| } catch { | ||
| log(.error, "Failed to serve \(href): \(error.localizedDescription)") | ||
| self.withTask { $0.didFailWithError(error) } | ||
| } | ||
|
|
||
| self.resource.close() | ||
| } | ||
| } | ||
|
|
||
| func cancel() { | ||
| assert(Thread.isMainThread) | ||
|
There was a problem hiding this comment. this looks a bit like a hack :) meaning, there's no reason from this point of view to demand that There was a problem hiding this comment. I initially implemented my own serial queue to synchronise the access to Basically it goes like this:
Now the trouble is, if we call
So like you suggested I tried to fix this by protecting the access to
At this point, we agreed with Apple's engineer that something was fishy in the This morning though I came up with this hack to resolve the situation without waiting for an hypothetic release: using the main thread as the "serial queue" protecting There was a problem hiding this comment. If I could fix
There was a problem hiding this comment. I see, now I understand. Would you mind adding a link to this discussion in the code so that the reasoning behind these choices is not lost once you merge the code? regarding the serial queue solution, would it be possible to schedule work on the serial queue There was a problem hiding this comment.
Sure, good idea.
The calls to But taking your other comment into consideration, I wonder if using the main thread as the synchronization queue is not the proper approach here. I considered There was a problem hiding this comment.
using the main thread to synchronize all the pieces is clever and it does work, so that's a big +1 and we should probably leave it in the commit history at the very least. That being said we are doing various work on the main thread that ideally we'd do in a background thread. I don't have a clear idea about how expensive that work can be: if it can't be very expensive, probably we could leave it on the main thread as-is and keep it simple. If it can be expensive, and since there are pieces of work that have to be synchronous and sequential, if we can rewrite this code in a way that we can post the chunks of work we need asynchronously to the serial queue, this would still preserve the sequential order and avoid deadlocks. One other equivalent approach could be to use NSOperations and chain dependencies explicitly but that's probably overkill. To summarize: using My understanding of There was a problem hiding this comment.
I think we're fine, there's no work done on the main thread except when calling back to Using the main thread as the synchronization queue means that we never have to explicitly wait for something from the main thread. If you look at the two delegate APIs (which are called from the main thread), they are not waiting on any queue: An improvement I could see is that instead of asserting we are (or not) on the main thread, we could schedule a main thread work when needed. This way if the r2-navigator-swift/r2-navigator-swift/Toolkit/WebViewResourceHandler.swift Lines 110 to 126 in 6c603de
Agreed but I don't think it's the case here. Do you have an example from the code? There was a problem hiding this comment. the only example I believe is the one in In any case, if we are sure that the work scheduled on the main thread is never going to be too much, I agree this should be fine. There was a problem hiding this comment.
I think the closures are pretty harmless, but I can't guarantee that
Yeah this was bad. If we ever go back to having a custom queue, we should do the approach we talked about earlier to replace the for loop. |
||
| self.isCancelled = true | ||
| } | ||
|
|
||
| /// Any API call to WKURLSchemeTask will crash the app if the task is cancelled by a call to: | ||
mickael-menu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// `webView(_ webView: WKWebView, stop urlSchemeTask: WKURLSchemeTask)`. | ||
| /// | ||
| /// To prevent this, we need to synchronize on the main thread every API calls to the task. | ||
| private func withTask(callback: (WKURLSchemeTask) -> Void) { | ||
| assert(!Thread.isMainThread) | ||
| DispatchQueue.main.sync { | ||
| if !self.isCancelled { | ||
| callback(self._task) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this literal be a constant on the super class, since it's also used in
EPUBReflowableSpreadView?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part and the one in
EPUBReflowableSpreadViewwill probably not look like this in the final version. I just set this up quickly to try out theWKURLSchemeHandlerapproach.