Skip to content

Conversation

lukeblevins
Copy link
Contributor

Under certain conditions, execution of blocks of code inside the file watching loop continues although the loop's condition is false, causing unwanted actions to be performed (e.g. using the closed directory handle)

This PR aims to:

  • Prevent a crash from occurring upon changing the working directory quickly
  • Remove unneeded checks before adding/removing files during the watcher operation
  • Convert any direct uses of the FilesAndFolders property to use a "snapshot" list instead

Please test this PR by doing the following:

  1. Removing/Adding/Renaming a file after refreshing the directory multiple times
  2. RELEASE MODE: Changing the WorkingDirectory by rapidly invoking various sidebar locations

@lukeblevins lukeblevins requested a review from yaira2 June 11, 2020 08:27
@ghost ghost added the needs - code review label Jun 11, 2020
@yaira2
Copy link
Member

yaira2 commented Jun 11, 2020

@duke7553 Is this supposed to fix the issue where the app sometimes crashes when opening a folder?

@tsvietOK
Copy link
Contributor

tsvietOK commented Jun 11, 2020

Bug with duplicated items still exists.
Sometimes getting an exception after item rename

2020-06-11 16:34:23.6512|ERROR|Files.App|Sequence contains no matching element|System.InvalidOperationException: Sequence contains no matching element
   at System.Linq.Enumerable.First[TSource](IEnumerable`1 source, Func`2 predicate)
   at Files.Filesystem.ItemViewModel.<>c__DisplayClass72_0.<WatchForDirectoryChanges>b__0(IAsyncAction x)
--- End of stack trace from previous location where exception was thrown ---
   at Files.Filesystem.ItemViewModel.WatchForDirectoryChanges(String path)
   at System.Threading.WinRTSynchronizationContextBase.Invoker.InvokeCore()
   at Windows.ApplicationModel.Core.UnhandledError.Propagate()
   at Microsoft.AppCenter.Utils.ApplicationLifecycleHelper.<.ctor>b__17_1(Object sender, UnhandledErrorDetectedEventArgs eventArgs)
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AppCenter.Utils.ApplicationLifecycleHelper.<.ctor>b__17_1(Object sender, UnhandledErrorDetectedEventArgs eventArgs)

throwed at line: RemoveFileOrFolder

case FILE_ACTION_RENAMED_OLD_NAME:
        RemoveFileOrFolder(FilesAndFolders.ToList().First(x => x.ItemPath.Equals(FileName)));
        Debug.WriteLine("File " + FileName + " will be renamed in the working directory.");
        break;

@lukeblevins lukeblevins changed the title Prevent the file watching loop from still being alive after we kill it WIP: Prevent the file watching loop from still being alive after we kill it Jun 11, 2020
@gave92
Copy link
Member

gave92 commented Jun 11, 2020

I'm not experiencing the duplicated elements bug anymore, but:

  • creating a new file
  • deleting it
  • hitting refresh

results in the same exception mentioned by @tsvietOK

2020-06-11 20:18:10.6754|ERROR|Files.App|Sequence contains no matching element|System.InvalidOperationException: Sequence contains no matching element
at System.Linq.Enumerable.First[TSource](IEnumerable1 source, Func2 predicate)
at Files.Filesystem.ItemViewModel.<>c__DisplayClass72_0.b__0(IAsyncAction x)
--- End of stack trace from previous location where exception was thrown ---
at Files.Filesystem.ItemViewModel.WatchForDirectoryChanges(String path)
at System.Runtime.CompilerServices.AsyncMethodBuilderCore.<>c.b__7_0(Object state)
at System.Threading.WinRTSynchronizationContextBase.Invoker.InvokeCore()

@lukeblevins
Copy link
Contributor Author

@gave92 Do you think a cancellation token would be appropriate to fix this one?

@yaira2 yaira2 self-requested a review June 12, 2020 19:16
@lukeblevins lukeblevins requested a review from tsvietOK June 12, 2020 22:59
@lukeblevins lukeblevins changed the title WIP: Prevent the file watching loop from still being alive after we kill it Prevent the file watching loop from still being alive after we kill it Jun 12, 2020
@gave92
Copy link
Member

gave92 commented Jun 13, 2020

@duke7553 The bug I mentioned above (creating->deleting->refresh->crash) appears to be solved 👍

Edit: [learning mode on]
- Can the call to WaitForSingleObjectEx never return?
-> Tested: it can, if you hit refresh multiple times you'll have many threads waiting for the next event
-> If you move to another directory the previous watcher will still be waiting for a change in the previous directory
- Why you are not closing the hWatchDir handle anymore?

Edit 2:
- I think you need to use CancelIoEx instead of CancelIo as CancelIo only cancels operations issued by the calling thread.

@gave92 gave92 mentioned this pull request Jun 13, 2020
* Cancel previous watcher

* Prevent Duplicate Execution of CloseWatcher()

Co-authored-by: Luke Blevins <lukeblevins15@gmail.com>
@yaira2
Copy link
Member

yaira2 commented Jun 14, 2020

:shipit:

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Jun 14, 2020
@yaira2 yaira2 merged commit f2c6487 into master Jun 14, 2020
@yaira2 yaira2 deleted the watcher-hotfix branch June 16, 2020 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants