FileDialog Fixes

[dodexahedron]

Part of the code in question, in FileDialog.cs:

void UpdateChildren ( ) {
  lock ( Parent._onlyOneSearchLock ) {
    while ( !_cancel && !_finished ) {
      try {
        Task.Delay ( 250 ).Wait ( _token.Token );
      }
      catch ( OperationCanceledException ) {
        _cancel = true;
      }

      if ( _cancel || _finished ) {
        break;
      }

      UpdateChildrenToFound ( );
    }

    if ( _finished && !_cancel ) {
      UpdateChildrenToFound ( );
    }

    Application.Invoke ( ( ) => { Parent._spinnerView.Visible = false; } );
  }
}

Problems:

• Several of the same sort of problems mentioned in #13, so I won't repeat those
• It almost uses cooperative cancellation, but then it doesn't really use things properly and still ends up locking and everything else.
  • This is a problem in many ways and is also confusing and inefficient.
• lock statement carries some unfortunate restrictions and makes this even more vulnerable to deadlock.
  • For instance, a lock statement never calls Wait or Pulse on the monitor to allow the thread to be used for something else while it waits.
  • Lock statements being waited on by another thread/object cannot be aborted. It's a call to TryEnter with no timeout.
• It locks a member of another object instance (deadlock-prone and breaks encapsulation)
  • Except for globals, an object not owned by something should almost never be locked by another object, even of the same type. That's deadlock city and has other unfortunate pitfalls that are hard to intentionally reproduce or write tests for.
  • Should be avoided if at all possible.
• The lock in use does not actually cover all places where relevant thread-safety issues can occur
• lock statements are turned into a try/finally block at compile-time, with a Monitor object used as the synchronization primitive behind it.
  • Monitor is a pretty expensive thing to use and is fully exclusionary, but doesn't do any good if everything touching the important stuff in the lock statement body isn't also using that same Monitor.
  • Not everything elsewhere touching the important stuff in the lock statement in this code respects the lock.
  • You don't have access to the actual Monitor, when using lock statements, meaning you can't even mitigate the earlier bullet about Wait and Pulse, since you can't call those methods with something you don't have a reference to.
  • Not only does the Monitor have to be Exited by the same object that entered it - It also has to be done on the same thread. This is the cause of the non-yielding thread blocking.
• If it ever deadlocks, you can't recover from it, since the object instance currently owning the Monitor is blocked and other objects are not allowed to touch a currently-owned Monitor.
  • Impact is not good no matter how it deadlocks.
  • Either you won't be able to exit the application or exceptions will occur when you do, potentially after a significant wait. In either case, this will appear as a hung task to the user.
  • The deadlocked object instance can never be cleaned up by the Garbage Collector
• For as long as the method blocks - including (and especially importantly) blocking on the Monitor from the lock statement - the thread executing it will be blocked and not useable or returnable to the thread pool.
• If code inside a lock statement ever calls any other code elsewhere that happens to lock on the same object, even more deadlock possibilities are created
• If code inside a lock statement performs an un-balanced number of explicit calls to Monitor.TryEnter and Monitor.Exit (could be in user code, too), you will either get a deadlock, some form of SynchronizationException, or premature release of the Monitor.
• If any other code uses the Monitor static methods to enter, exit, wait, or pulse, code using lock statements will never know about it.
  • This essentially means that, if you use lock in one place for a particular mutex, ALL other uses of that mutex should be lock statements or else things can get very unsafe, very quickly, very confusingly, non-deterministically, and in a very difficult to debug way.
• This use doesn't even actually result in thread-safe behavior on all critical code, because there are reads and writes to certain members in other places that do not perform any kind of synchronization whatsoever, exposing them and the code under lock to several of the classic thread-safety problems.
• The object used for the mutex is mutable. Needs to be readonly so the reference can never be reassigned.

And more, but I'm done for now and want to actually get some work done.