fix preview functionality on windows #155
Conversation
|
Looks good to me! I wonder if we should limit the number of retries to prevent infinite looping? Could it be possible that a permission exception is thrown for some other reason? |
|
Sure, I'll add a way to limit the number of retries, after which I suppose it should just exit? With respect to another case of the permission exception being thrown, I'm not sure to be honest. I suppose a permission denied error can also be thrown for actual file permission mismatch issues, like attempting to open or read a file for which the user does not have permissions. The problem is I don't know what the best way would be to disambiguate the two, or if there even is a way other than knowing the context in which it occurs (i.e. preview component). I think that if there was another case (most likely actual file permissions mismatching), it would most likely be exposed by This would be one advantage of using the polling interface I suppose, since it wouldn't introduce ambiguity in exception handling of the permission exception. I'm going to experiment with using the polling interface to see how it performs. It shouldn't be difficult to switch to using that. |
|
Does win32 API emit multiple modified events in case saving process is running for a while or does behaviour vary depending on application which is writing the file? If events are remitted, then the whole retry business can be dropped as the event would be repeated after .1 second (because of debounce). Simple would suffice. It also could be applied to all platforms unconditionally as 0.1 second delay before the build is not too much and I myself prefer clean code. Otherwise 👍. |
|
@simukis To be honest, I haven't a clue (I have little to no experience with the Win32 API), but throughout my investigation of the issue I get the impression that it does not, given the frustration of people who used the API. I had indeed tried a simple delay before, of the same duration you mentioned, and I just tried it again. It does work most of the time, but every once in a while I suppose there's some sort of timing issue and the permission error presents itself. When this occurs, the whole preview system grinds to a halt. By the way, I looked into the possibility of forcing fs-notify's polling fallback but it seems it provides no easy way (if any) to accomplish this. I'll look into it further later. |
|
I have touched-up the commit a bit:
It seems to work very well. Let me know what you think. EDIT: Like I said in my previous comment, I was going to try to make Hakyll use fs-notify's polling fallback (instead of the native OS notifications API) but fs-notify doesn't seem to expose a method for doing this. It uses OS specific APIs for Mac OS X, Linux, and Windows, and if the OS is something other than these then it uses the polling fallback. fs-notify doesn't seem to expose the types/functions required to force the use of the polling fallback. That said, the workaround I came up with does seem to work very well. |
|
Awesome work, thanks! |
fix preview functionality on windows
As #153 states, preview functionality is currently broken in master due to the merging of @simukis' code in 5f6035b . The changes he suggested are overall better, in my opinion, as they utilize operating systems' specific file event notifications APIs. The problem is that on Windows, the API often used for this reason -- and the API consequently exposed by fs-notify (by way of win32-notify) -- has a flaw/difference in the way it handles file modification events.
In essence, the problem that is experienced on Windows is that the file modification event is sent/received before the program doing the modification is even done doing the modification (i.e. before it is finished writing to the file). As the code currently stands, Hakyll immediately upon receipt of the event attempts to read the file (in order to generate the changes) and the operation fails with a 'permission denied' error because the program (e.g. editor) still hasn't relinquished writing control of the file.
For more information, please refer to this post in the issue I created for win32-notify, which summarizes my findings.
As is mentioned there, I don't think this is a flaw in win32-notify, as it's simply exposing the API more or less verbatim in its bindings. As the post shows, this is simply a recurring problem encountered by people using the
ReadDirectoryChangesWAPI. I don't think it's something that can be (or maybe even, shouldn't be) implemented in win32-notify because that would embed the assumption that all users of the library want/need to open/read the file for which the event occurred. It could be they're simply logging events to some other place.What I found was that the common workaround for this is to simply keep trying to open the file until it is possible to, then continue with the operation dependent on being able to open/read the file. This is what I have done.
This inevitably introduces CPP conditionals which I've tried to keep to a minimum, and as neat/clean as possible. The way the workaround works is by attempting to open the file, and if a permission denied exception is raised, sleep for 100 milliseconds before attempting once again. I've been using it on my end and now preview functionality works perfectly, and is quite fast.
The only other alternative I can imagine is to force the usage of the polling interface that fs-notify exposes, though I haven't tested this myself. This would revert preview behavior to pre-fs-notify state. The disadvantage is that all files in the provider directory would be constantly polled, instead of a more 'push' notification-like behavior when using the actual operating system's file event notifications API. It's true that this workaround is a bit similar to polling, but it would only be occurring for the file for which the event occurred, instead of for every single file.
Let me know what you think.