Support for File Provider file access#629
Conversation
|
Interestingly I've also recently been investigating asynchronous processing of native messages. I was wondering if you have tested your branch? Because I found it doesn't compile in Xcode. The reason is that I haven't reviewed your changes carefully. But I have a curious question, if we wait for the download of files in iCloud through an asynchronous method, will this waiting significantly delay the loading of the extension UI and lead to even later script injection? |
|
|
||
| Task { | ||
| guard | ||
| let request = context.inputItems[0] as? NSExtensionItem, |
There was a problem hiding this comment.
Capture of 'context' with non-sendable type 'NSExtensionContext' in a
@Sendableclosure
There is a warning here, but I believe this is a difficult issue to reconcile in Swift's asynchronous.
There was a problem hiding this comment.
Yeah, I was looking for something like Rust's block_on but it seems like Swift really doesn't want you to do something like that, and it also seems like you can't control which thread an async task runs on. Hence this awful code. It should be safe though, considering the semaphore should ensure the object is only handled by one thread at a time.
There was a problem hiding this comment.
It doesn't seem as safe as one might think: https://stackoverflow.com/a/71971635
But there seems to be no obvious problem in this scenario, since there are no repeated calls in the logic, and there should be no problem even without using a semaphore.
There was a problem hiding this comment.
Right, this answer seems to me to specifically be about calling a blocking function inside an async context (from a thread used by the async runtime). Doing that can lead to a deadlock in probably any language with async functions, including Rust, if you have limited threads. I'm pretty sure beginRequest(with:) here is not called from an async context, though, so none of the async runtime threads will ever be blocked.
The semaphore is just in case the function calling beginRequest does something else with the NSExtensionContext afterwards. I don't assume so, it did work without it but dealing with data races in the past has made me careful :P
There was a problem hiding this comment.
After revisiting this incidental issue with more understanding, it does not seem difficult to resolve.
The warning here is that the non-sendable context: NSExtensionContext variable is passed to the child thread, but in fact we could wait for the child thread to return the response data (which is sendable) and then call context on the main thread.
In other words, illustrated through code:
func beginRequest(with context: NSExtensionContext) {
+ var response
let lock = DispatchSemaphore(value: 0)
Task {
guard
let request = context.inputItems[0] as? NSExtensionItem,
- let response = await asyncBeginRequest(request: request)
+ response = await asyncBeginRequest(request: request)
else {
return
}
- context.completeRequest(returningItems: [response], completionHandler: nil)
lock.signal()
}
lock.wait()
+ context.completeRequest(returningItems: [response], completionHandler: nil)
}
Oh oops, that's iOS code. I only tested the macOS build so I didn't catch that.
Yes, right now this is basic support, this will absolutely block loading the script list until the files are completely downloaded. It behaves the same as over any potentially slow FS (such as webdavfs or nfs) In fact, those are probably orders of magnitude worse since getAllFiles gets called a lot and at least I'm pretty sure nfs will read the file from the server every time. I recommend proactively loading the files and watching that directory for changes, only then reloading, instead of re-reading every time. |
Reading files from File Provider file systems such as iCloud using the normal blocking API will fail with EDEADLK 'Resource deadlock avoided.' if the file is not currently locally downloaded. Therefore, do file access asynchronously inside NSFileCoordinator coordinate(with:queue:byAccessor:) to avoid this.
dblsaiko
force-pushed
the
outgoing/fileprovider-compat
branch
from
751a67f to
66eb8db
Compare
I think this might not be an appropriate way to implement it in this extension. I think the appropriate way to do this might be to notify iCloud to download those needed files and return the existing ones. Include the contents of those newly downloaded files the next time if they complete the download. Maybe use this API: |
Yes, this is a known issue and one of the main back-end refactoring faced. |
That's what already happens right now though. None of the behavior changes (except for concurrent reads which I added because it was straightforward), it just additionally makes it work for File Providers. It will currently also block if the file system is slow for whatever reason, that's kinda the nature of blocking I/O function calls.
With that, a script may not be injected at all, as opposed to injected maybe a couple seconds late (which may, of course, happen for other unrelated reasons as well including simple system overload). How is never better than late? I'm absolutely not saying this is best solution, it was intended to be a quick fix so that I could put the scripts on iCloud without having to be paranoid about them disappearing. The real fix requires re-designing the script loader to cache scripts in memory. |
|
That's an unfair idea, please calm down and think about it carefully. I was excited to see your solution and even spent several more hours investigating the async issue. But then I found that question that made me curious, if adding async support would further increase unexpected latency, which may not be what we want. I don't mean async concurrent reading of the files, I'm not sure what the energy efficiency change would be. All asynchronous processing will add uncontrollable latency, including of course those I/O processes. But to be fair, reading files from a local hard drive (usually an SSD) is pretty fast, at least much faster than downloading from iCloud (depending on the network). Like you said, reading the file from the local hard drive could be synchronous (even if it's in the iCloud path but has been downloaded), whereas waiting for the system to download from iCloud requires asynchronous support, which could be a significant time difference. The crux of the issue lies in iCloud's eviction mechanism.
To be more clear, if there are 5 scripts in the iCloud path, 3 of them are not local due to the iCloud eviction mechanism. That's what I'm trying to say. I'm not talking about extreme efficiency (using memory), And I'm not sure the complexity is worth it. |
|
I'm sorry if I came across as being mad. I just think what you're proposing is a trade-off that would bring back the original issue that I have (though at least in a less annoying way), and also doesn't ensure that injection never delays because of FS access, so it's not really something I'd want to implement. These are kinda my two points:
In my opinion, the preferable way to fix both of these at once is by not putting disk I/O in the script injection code path. I can definitely solve this though, it might just take a bit longer. What I have in mind shouldn't be that complex, I just need to get familiar with Swift and the OS APIs. I'll mark this as draft in the meantime. |
|
I basically agree with you. And I have no experience with Rust, so I assume you are more expert than me in some aspects. It should be noted that we are not need to solve all related issues/processes in this issue/PR, It's just about iCloud access. My main point with this PR is that putting the iCloud download process into the UI loading/script injection code path creates new delays. Yes, although it seems solves the issue of iCloud file access. And due to the impact of uncertain network quality, this delay can be very terrible. We never want to see it spinning in circles for 5 seconds or more when opening the UI while waiting for iCloud to download, right? So combined with that top-level async problem that can't be solved perfectly, I don't think we have full confidence in adopting this implementation for now. Do you think so? This is also why I thought when I first raised the issue that we might need to consider fully supporting iCloud and redesigning the synchronization process to better address this issue. Because using CloudKit apis we can control the iCloud sync process and avoid eviction of required documents. (I haven't investigated in depth and am not sure if the same with iCloud Drive) |
It seems that things are different from what I imagined. It can be seen that the correct and feasible approach may be to copy the files to the App Sandbox document and update them through some synchronization mechanism. |
|
I don't know if this could be a possible simple fix with no noticeable side effects, just check via Based on frequency of use, it may be less likely to be evicted from the system, and it also facilitates synchronization with the cloud version.
While this by no means completely solves the issue, and may not appear in the extension during the first few requests. However, it may significantly reduce the edge case of complete unavailability due to eviction for lack of space. Of course, the better approach would still be to completely redesign the storage and synchronization logic, which would be a huge development and refactoring effort. And I can see that the correct storage and synchronization mechanism does not require us to have a top-level asynchronous architecture, because we do not need to wait for the data returned asynchronously. If you're so inclined, try this out and see how it performs in daily use. Since I don't currently use iCloud for storage and syncing, I can't observe and test its actual performance. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
According to the information, the new "Keep Downloaded" feature in macOS 15 / iOS 18 almost solves the main issue. But it seems we may still encounter other brief periods of inaccessibility. It seems that loading the scripts directly from the iCloud folder is not a feasible solution, it involves too much uncertainty and delay. To completely solve this issue, it is best to design an asynchronous mechanism to synchronize user scripts to the sandbox container to ensure continuous reading. I think this will happen in Milestone v6.0.0, a overall redesign and refactoring of the backend process. @2xsaiko Your attempts and efforts are still greatly appreciated. Thank you! Given that we will not be adopting the solution and going much further, let me close the PR. |
I've been using that for a while and it seems to work well for me, yeah.
Ah nice, an internal cache would also ensure the files don't go away (e.g. when then user manually evicts them from the iCloud cache, or even when they're on an external storage that isn't always connected). Sounds good! |
|
@2xsaiko
Is this the result you got from the extension at the time, or is it an external test? Although I have never reproduced any error logs, based on user feedback, I speculated that the problem occurred earlier, i.e. In other words, we simply cannot get the path to the iCloud folder. Since we are the sandboxed App, without the resolved path, we cannot access any paths outside the sandbox. Not to mention trying to read files asynchronously. In other words, unless we use the iCloud container of the App sandbox itself, there seems to be no other solution to the issue except using the system's "Keep Downloaded" feature. Sorry I asked the question again after so long. It's just that when I got more understanding, I found that this didn't seem to match your previous attempts. If you could recall some details of the time, I would be very grateful. |
|
I might have tested this with a small Swift or Obj-C program before writing this to figure out what the error was, not sure. However I did test this change in the extension and it did what I expected it to when selecting the folder inside iCloud via the settings, downloading the files when accessed. I'm not sure whether I hit the folder path disappearing when it got evicted or tested anything like that though. |
|
@2xsaiko Thank you for your addition. In fact, I'm not sure what the system is doing to evict iCloud files or folder. But several users have already reported that the iCloud folder selected will be reset to the default directory periodically, and the only logic in the code to reset that directory is that So I'm not sure if the iCloud eviction folder will directly cause the So far I haven't seen any valid error logs. I hope this assumption is not true, otherwise even implementing caching will still face issues. Thanks again, stay safe. |
3 participants
Reading files from File Provider file systems such as iCloud using the normal blocking API will fail with EDEADLK 'Resource deadlock avoided.' if the file is not currently locally downloaded. Therefore, do file access asynchronously inside NSFileCoordinator coordinate(with:queue:byAccessor:) to avoid this.