dodexahedron / terminal.gui Goto Github PK

The goals in https://github.com/users/dodexahedron/projects/12 and https://github.com/users/dodexahedron/projects/10 will be easier to achieve if some fundamentals are tackled early, especially with an eye toward both providing easy and open flexibility while also keeping that scope in check.

There is technical debt scattered around, both from legacy code and new code (often as a totally understandable consequence to wanting to get a given unit finished), but hey - I don't mind doing this kind of boring work, so here we go.

First, I'll just mention that this work is going to start from the branch I originally made for #22 (currently not pushed to GitHub), because problems there are what finally prompted me to confront this potential beast.

This issue will probably encompass a bit of a smorgasbord of individual changes, but we'll see what I end up actually doing as work progresses.

Initial Intentions for this Issue

In the scope of this particular issue, what I'm thinking is that we can probably use both some new interfaces and members on existing types and interfaces, as well as potentially expand the capabilities and definitions of existing interfaces and abstract types.

Starting scope of work

The initial scope, which is not very likely to remain well-constrained, is the Application class and types and functionality it directly depends on.

If changes in any specific dependency seem to be getting beyond what's reasonable not to be mentioned solely in commit comments, I'll either say something on an associated issue or PR, or else I'll fire up a new issue, if something starts to grow beyond a few incidental changes.

Desired end goals for this issue

• New and/or revised interface definitions and implementations around the Application class and types it is closely associated with.
• Separation of types to their own files in files that have non-negligible work done on them.
• Modification of parts of the public and internal API surface in Application and the ConsoleDriver-related types to enable wider use of interface implementations.
  • A ton of reasons for this, not the least of which are easier, cleaner, and safer (type-safety, thread-safety, etc) code both internally and for consumers.
• More, but I want to get back to work! 😆

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.

Lots of reasons for getting rid of this type and similar types.

The main difference that actually affects anything is that it enforces values >= 0 in property setters (and also implicitly in the constructor, because of that) and throws an ArgumentException on negative values.

That alone is enough reason to at least refactor the type, but may as well just remove it.

Why?
Well, first my semi-subjective reason (though it's based on the objective stuff below):

• Subjectively, I'd argue that it's a bit too far to the "nanny code" side of things, and that we shouldn't do that on a primitive.
  • Rather, if a type using a Rectangle has constraints on that Rectangle, it's that type's responsibility to check those constraints if/when/where appropriate/necessary/prudent, just like it would be that type's responsibility to validate anything else given to it.

Objectively:

• It's against language design guidelines for exceptions, both in property setters and in constructors, both of which are strongly suggested to be avoided if not absolutely necessary.
• It's not a proper type of exception to throw for that operation, even if it were a good idea to throw there. InvalidOperationException with an optional inner exception is the standard. ArgumentException shouldn't be used for non-indexer properties because, in C#, there's no concept of an argument to non-indexer properties.
• It places a restriction on the type that is only relevant to specific dependent code.
  • That tends to be a particularly difficult bit of coupling to deal with, meaning it's very oversized technical debt for such a seemingly innocent and well-intentioned bit of code.
  • Other code which may not care or for which those values may be perfectly legitimate is bound by the restriction, which leads to kludges, additional types, needless tuples, etc.
• There's nothing inherently "wrong" with a rectangle with negative properties. It just means the shape is flipped or translated about its origin with respect to whichever values are negative. And that's the consumer's problem, not ours.
  • If we can't draw it because of something like that, we need to validate the rectangle at assignment or usage, or otherwise need to handle bad values in a safe way, such as:
    • Treat the invalid dimensions as 0
    • Translate a "bad" rectangle to a "good" rectangle, using the Offset method, to turn it into its equivalent all-positive-value representation, if one exists, or getting the intersection of that result with its parent's Frame or whatever if still out of bounds. This would be a good place for an extension method, if that functionality is desired.
• The behavior is different between Rectangle and RectangleF, with RectangleF not enforcing the same rules.
  • This could potentially lead to several different error cases, some of which may not even be directly the consumer's fault and some of which might not be directly fixable by the consumer (without a kludge or potentially even at all, in theory).
• The behavior isn't consistent with the base class type that this type is otherwise mostly identical to. This fails the "don't be weird" test because it's a subtle but breaking behavioral quirk introduced here that users shouldn't have to expect to deal with and will probably get annoyed by, if/when they happen and take down their application as a result.
  • Much better to just draw nothing and maybe include a hook for a debug log whenever the values are used, if the method using them has to fall back to 0. Otherwise, just let the library code that consumes the rectangle throw a relevant exception if necessary.
• Inflate() leaves things in a corrupted state if Width is valid but Height is not, after the multiplication, or if the assignment to X or Y result in overflow.
  • There's even a comment that suggests it's safe the way it does it. The comment is only true if setting Width throws the exception.
  • System.Drawing.Rectangle.Inflate does it in an unchecked block for multiple reasons, not the least of which is to avoid throwing an exception from the property setter and allow all 4 operations to complete in a consistent manner even if the caller is boneheaded.
• It's a behavior of a primitive that has to be tested in that primitive and then that all dependent types still have to test for and have explicit exception handling for or, at minimum, documentation of the possible exception that could be thrown. The cause of such exceptions could have sources that are difficult to track down, especially since these are runtime values that can, do, and are likely to change quite a bit over the lifetime of an object owning one of these types.
• It's something a consumer might be understandably tempted to just define a conversion for, so they can use the built-in types themselves, especially if their application already does. But that's a lossy conversion in that the two types have different behaviors.
• The behavior is not documented.
  • While that's as easy to "fix" as adding an <exception... XmlDoc tag, the above I think supersedes that, especially since these particular types are so basic and so simple to replace.

Speaking in general and not specifically about these types:
If it's really that critical to do things like that, in that sort of situation (enforce bounds on primitives of types that also exist in the BCL), then it's more appropriate to do the following:

• Wrap the BCL type in a type explicitly and significantly differently named (Like Quadrilateral instead of Rectangle, as a crude example) so as to avoid typos or errant autocompletions even being possible.
• Expose all the normal functionality of the wrapped type on the wrapper type
• Implement whatever additional constraints/behaviors are desired in the wrapper.
• Loudly and clearly document the behaviors and the reasoning for them.
• OR, instead of those items, re-think things, because it's probably indicative of a problem in design or methodology.

But this one is very arguably not an example of that kind of situation.

For reference, the current dotnet source code for the System.Drawing types I intend to replace is in these files:

• System.Drawing.Rectangle dotnet runtime source
• System.Drawing.Size dotnet runtime source
• System.Drawing.Point dotnet runtime source
• System.Drawing.RectangleF dotnet runtime source
• System.Drawing.SizeF dotnet runtime source
• System.Drawing.PointF dotnet runtime source

Local fork issue to track work on gui-cs#3240

As I just mentioned in #13, there's an opportunity for some nice cleanup and code quality improvements around the Windows native methods we call via PInvoke and the types that support them.

The NuGet package Microsoft.Windows.CsWin32 is a source generator package that just takes a simple text file input and creates all of the methods and the types they depend on.

I've played around with it a little, already, and it is great!

Since I'm already tearing into the WindowsDriver and associated types, right now, I may as well do this opportunistically.

This issue and its associated branch are for that work.

As alluded to in #22, there are going to be a bunch of issues needed to address concurrency issues that are causing a wide variety of sporadic and nondeterministic problems at runtime and in unit tests, with increasing frequency.

The changes necessary to fix them are not terribly difficult, and they happen to also overlap to a pretty significant degree with work I am already doing on other branches, such as the event handling refactoring.

This is more critical to be tackled first, for a lot of reasons, so I'm starting with where I was already working on null fixes: the Application class and types it directly interacts with.

More to follow....

Need to take a pass over the code to find and fix any remaining null checks that are of the form x == null or x != null, since R# wasn't able to automatically take care of all of them in the reformat work.

The HACK_CHECK_WINCHANGED is used to conditionally compile a hack to make Windows behave better.

However, it is defined in code and thus will always be set.

Let's set it in the build configuration.

Since Rect is just a copy/paste of Rectangle, aside from slightly different ToString behavior, it's a prime candidate for removal.

To make removal smoother, I'm going to approach it in two stages. First, I'll make the behaviors it exhibits that are different from System.Drawing.Rectangle the same as in that type. That will require fixing unit tests, since the ToString output is heavily relied upon in many tests.

Once everything is working, the type can be renamed, to make the solution-wide change to Rectangle. Then the type can be deleted and a reference added to System.Drawing.Rectangle as a type alias (rather than the entire namespace) to swap them in-place.