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
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.
2xsaiko
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. |
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.