Refactoring, cleanup, and abstraction for ConsoleDrivers #3692
Comments
dodexahedron
added
bug
enhancement
work-in-progress
Windows
design
|
Oh another biggie I forgot to stick in the list of stuff that's already (mostly) done, and which falls under more than one of those top-level categories: The code around use of unmanaged resources like the console file handles used with SetConsoleMode and such has been reworked to use actual SafeHandles (FileHandles, primarily) to enable better/safer/easier use of them, including proper lifetime management/disposal. That's important because Terminal.Gui's lifetime is in no way tied to the lifetime of the entire application (which is another reason I am particularly against unnecessary use of statics). As a real-world case of that concept: My SnapsInAZfs application, which led to me using and then ultimately contributing to TG, ONLY uses TG in a specific scenario, and otherwise the application typically runs as a headless service process or a one-shot process that has no need for a TUI. It's only used for a configuration interface. Even unit tests expose some of the pain points there, which we've mostly worked around in other ways, but still don't fully address because we are implicitly taking advantage of the fact that test runner processes mostly do have TG lifetime (almost) equal to process lifetime. The point is that we can't treat TG like it is going to be in use from start to end of the application. That makes proper handling of things like unmanaged resources pretty critical to not make any assumptions around. |
|
@tig, a win32 question for you: Have you ever tried or considered creating additional handles to the stdin/stdout/stderr file descriptors at startup time? That method can be used to grab our own copy of the original file descriptors, which can then be used for the lifetime of TG, regardless of what the consuming application does. Why do that? It would fix cases where one or more of those are redirected after the application is launched, which currently causes a crash. |
|
@tig, a unicode output question for you, RE: the NetWinVTConsole class: It has the same guard against non-BMP runes as the CursesDriver does. Why is that there? The windows console can display any UTF-8 character your font has a symbol for, including surrogates. Behold: // This code:
Console.OutputEncoding = Encoding.UTF8;
ReadOnlySpan<byte> bytes = "👨🏻👨🏼👨🏽👨🏾👨🏿"u8;
using Stream stdout = Console.OpenStandardOutput();
stdout.Write(bytes);
stdout.Flush();
// Has this output:
|
|
Also works fine for UTF-16: Console.OutputEncoding = Encoding.UTF8;
ReadOnlySpan<byte> bytes = "👨🏻👨🏼👨🏽👨🏾👨🏿"u8;
using Stream stdout = Console.OpenStandardOutput();
stdout.Write(bytes);
stdout.Flush();
Console.OutputEncoding = Encoding.Unicode;
Console.WriteLine();
Console.WriteLine();
Console.WriteLine(Encoding.UTF8.GetString(bytes)); // Conversion to string is UTF-16. The UTF8 here is the SOURCE encoding.
|
|
For reference, the UTF-8 version of the string is 44 bytes, and the UTF-16 version is 20 chars (40 bytes), in case there's any question of whether those 5 emoji are multi-byte/surrogates or not. |
|
There are a ton of nuances including versions of WT. it's possible they fixed Atlas since I last worked on this. Happy to have it finally fixed. But I'm skeptical. If you think you have a fix please test with CharMap. |
|
Yeah I suspected that might be the sort of sticking point. Mainly, I wanted to either be able to remove the IsBmp check on that or put a remark on it like the one in CursesDriver, for now, since it's currently without comment in that driver. This work isn't aimed at changing our unicode handling. That, if/when it comes, will be in a completely different change set. We do have ways of continuing to drastically reduce allocations and copying and all that around handling of text, but that will also come in time and requires a lot more fine-grained testing than the sort of changes I intend for this and subsequent change sets in the series. |
|
Update: I Decided to go all the way with the PInvoke improvements for the files being touched in this round. I let CsWin32 generate the initial interop types, and then I took the generated code and refined it to what you'll see in the PR. I then removed CsWin32 since further benefit from it only really exists if adding new PInvokes, specifically for windows APIs, so may as well not make builds take the extra couple of seconds. 🤷♂️ These involve no run-time code generation and only use blittable types for the platform calls, so are thus theoretically fully trim-safe. I've still got a bit more left to do with it, but I'm working on it right now. |
Huge. I started doing this way back in #2490 but ran out of steam. You doing this will help that effort immensely! |
|
Well, and, as part of it, I compared code generated by CsWin32 against the code1 generated by use of LibraryImportAttribute. CsWin32 tends to generate leaner code in some cases, but it also doesn't really understand some nuances of the underlying APIs, it seems, or at least for whatever reason does not generate quite as robust code around the marshaling of things between managed and unmanaged as LibraryImport does. They are both source generators targeted at broad usage and compatibility, so of course they both generate slightly sub-optimal code in their own ways. So what I'm doing is taking cues from the output of each, paring it down to what's actually relevant, cleaning things up to be legible, and whatever other tweaks make sense for these specific API calls and our use of them. I'm pretty sure I mentioned a long time ago, in another pinvoke-related issue, that LibraryImport doesn't actually fully replace DllImport. DllImport is the mechanism used for PInvoke, at the end of it all, even for source-generated interop. It's just that, if you use DllImport without ensuring that everything has already been marshalled correctly and everything is blittable, the compiler ALSO emits a stub into the IL that triggers run-time generation of the code necessary to do all of that, and that's what is incompatible with trimming, because that's all Reflection.Emit and such. One thing CsWin32 does that looks, on the surface, like it might be a nice advantage, is that is also generates structs to match native types like HANDLE. Cool, right? Well, yes, in some cases. But not this one. There are already built-in types specifically for this stuff. In particular, SafeFileHandle and its associated types are much more robust way of wrapping the handles involved. The most basic operations they perform are pretty much the same - wrapping a It also implements IDisposable and proper finalization, which isn't possible with a struct, because they do not participate in anything related to the GC (the other definition of "unmanaged"). However, CsWin32 did generate some helpful things, like enums for the various DWORD constants used for methods like SetConsoleMode and for the values of the STD_HANDLE constants for stdout, stdin, and stderr. Those things are handy, but aren't generated automatically by LibraryImport, since it only uses whatever types you specified in the partial method signature. So, I'm keeping those, since they're simpler and more expressive than having a bunch of constants defined, and easier to use when calling things. For any interop type added, I went to the documentation for those APIs and grabbed the relevant portions to stick into XmlDoc comments so we have a nice and fully-documented set of stuff to work with, without having to constantly go back and reference MS Learn. CsWin32 makes an attempt to do that, but what it creates is REALLY messy and is just raw markdown taken from the github source for the MS Learn documents, which of course doesn't work well in XmlDoc comments. So, it's been a little bit of a roller coaster with things being introduced, removed, re-introduced, massaged, etc., so I'm distilling the commits down to a lot fewer, mostly just dropping some that ended up being irrelevant noise in the end. Another bonus to SafeFileHandle: it is cross-platform. .net has platform-specific code to make it work consistently regardless of operating system. That also opens up possibilities for more code reuse across the different drivers, since that is an abstraction that can then be pulled up to a less-derived type in the hierarchy - potentially even all the way to the base ConsoleDriver class. I am not doing any of that in this change set, however. This is just laying the groundwork. Footnotes
|
|
OK, sooo... I had a thought based on the win32 API calls used to interact with the console. So yes, there's the handle. But...Why is it a SafeFileHandle? Because it IS a file handle, and always has been since the DOS days. The device "files" (same concept as how unix treats it) are streams named And it does it via CreateFile. And the documentation explicitly says that's a way for you to grab a handle to each. So I figured ok... There are already .net implementations of all of those APIs. It's only the Set/GetConsoleMode that don't exist as far as I can tell (as far as our usage of the console APIs goes, that is). So I tried it out. And it works beautifully and also provides a way for us to fix a bug that we have basically just tried to work around by dealing with excptions, in the past: the problem of someone redirecting the console streams. If you grab a new handle to the desired stream via Since doing things that require PInvokes that need handles to console streams appears to be a common question about, around the internet, I figured I'd write it up in a more public place than here, with explanation and code sample. The only answers I saw anywhere always immediately resorted to PInvoke, which tends to lead to at several PInvokes, which this method avoids a few of. It is provided as this answer on Stack Overflow. It's going to cut lines of code in this PR down by a fair bit. And any chance to avoid crossing the managed/unmanaged barrier in non-native code (ie our code) is a win, as well. |
|
Continuing the above... The same concept is valid on unix as well. After all, in unix-land, everything is a file - files, sockets, devices, and even you are all files. The console for the current process is /dev/tty, and it is a bidirectional character device, meaning you only need one FileStream to both read and write to it. If someone redirects stdin or stdout in the program, your FileStream you opened to it is unaffected, just like in Windows. Unix systems typically also expose sockets that are essentially just symlinks to the console tty device, but in testing among different distros, versions, and shells, the specifics of that vary wildly enough to not be very attractive to try to figure out, though a "socket" abstraction is still basically the same idea, since it's still just a file like everything else. 🤷♂️ |
|
And of course they're fully capable of ANSI sequences, just like the original stream from the static methods on using FileStream consoleOut = File.Open ("/dev/tty", FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite);
consoleOut.Write ("\x1b[38;5;190m\x1b[48;5;4mI'm writing colored UTF8 directly to the console!\x1b[39;49m\n"u8);outputs:
The above was run on an Ubuntu 24.04 system, via ssh, connected from Windows Terminal 1.20.11781.0 on Windows 11 24H2 26100.1457. |
|
Side note: Man, I wish they'd back-port the Something that might make sense for us to do to enable easy switching to that in future versions is to define a constant like If put into a conditional as follows, it would automatically happen if compiled under .net 9 at least: #if NET9_0_OR_GREATER
const char ESC = '\e';
#else
const char ESC = '\x1b';
#endif
const string ESEQ = "${ESC}["; |
Why just not using the public const char KeyEsc = (char)KeyCode.Esc;
public const string CSI = "\u001B["; |
|
Yes, that's the exact idea. I just forgot we had that, in that moment. 😅 In any case, the conditional to switch it out for |
One additional potentially major upshot of this is it MAY offer a path to lift our curse(s), at least partially. 🤔 |
|
And note that the utf-8 string in that sample is not a string. The u8 suffix makes it a One unfortunate shortcoming of the current implementation of that is that string interpolation and u8 are mutually exclusive, even though interpolated strings can themselves be constants. 😞 |
|
oooo but I just had an idea... Might be a way to achieve something similar. Gonna try it out later. This is just a reminder to myself, for when I get back, so I don't forget the idea: u8 + spread operator + collection initializers? |
|
@tig, a question for you: How many buffers would you like things to be using? Are we wanting to be double-buffering? Triple? More? We can probably afford to go pretty deep if we want, since we're dealing with fairly small screen space, so long as we refine some things like the PInvokes and the marshalling around them. As I get into the weeds of this, I'm realizing there are some simple yet important things that can be done in modern .net and c# to drastically reduce the hammering on main memory and cache flushes that come along implicitly with how some of the old stuff works, due to memory barriers and other goodn't things that happen quite bit but which can now be mitigated, and in what end up being remarkably simple ways. |
Previous to your work here, and my work on ConsoleDrivers last year, it was a true mess. What I focused on was correctness and simplicity of the API enabling reduction of duplicated spaghetti code in the the drivers. I am not YET concerned about performance. Until we get wide-char rendering and truecolor working on all drivers, AND we have a driver that works purely with ANSI escape sequences the concept of how many screen buffers we have is still in flux. Thus, my recommendation is to hold off unless you have absolute clarity of how the above will be addressed within the scope of your design. IOW: I am biased towards architectural/API cleanliness, and you are biased towards performance. We both tend to each a bit too hypothetically. I believe the tie-breaker should NOT be hypothetical performance issues. ;-) |
|
I talk about performance a lot because that's just still pretty low-hanging fruit for us, here. But the main goals I have nearly always originate from architectural improvements and quality of life improvements for both the consumer and their end users. Performance just goes hand-in-hand with it a high percentage of the time when one can write the same operation a little differently during that work and avoid at least a chunk of what made that particular unit expensive. And around PInvoke, those pitfalls are so numerous (in .net applications in general, just because of how all of that stuff works) that it starts to look more like a crater rather than individual pitfalls, and you just happen to have a giant dirt pile 3/4 the size of the crater right next to it ready to push in with a bulldozer. As it turns out, a lot of what I'm doing in this work enables a pretty large amount of code re-use between the different drivers, which I know you'll love, and which was another of my main goals when sizing up what I wanted to work on for this change series. Performance just happens to be closely related to the things getting cleaned up, which makes me even happier and more motivated. Plus it's nice to get a tangible result. I've always liked improving products, tools, processes, etc every bit as much as creating new ones. Sometimes even more, since I actually have fun trying to get inside someone's head to see if our combined efforts can result in something even better in the end. 😁 Another of my goals for this that I'm pretty sure will make your day once it's ready is to completely eliminate more than one pretty large internal class, due to the cleaner architecture, by eliminating a ton of duplication and then merging what's left since even in their current states they already overlap quite a bit (for various reasons - organic growth can get that way sometimes). So, if the performance wins we will get from the work I'm doing anyway don't pique your interest, then I'll slightly re-phrase the question: The architecture in the console drivers is much more complex than it needs to be and can be greatly simplified by doing a few relatively simple things.1 One of those things requires me to be briefed on some intentions, or else it'll just be me doing that work unilaterally and then going "SURPRISE!" I would rather get your input, in case you had any insights, plans, prior experiences to watch out for, or even just preferences before I pick a design for that. Just to save a back-and-forth: As a side-note2 slightly more than tangential to the topic (secantial? reentrant? 🤔): I tend to play the long game, strategically. Even though some of the things I'm doing right now will have very visible results to anyone working on the code of TG itself, some of it lays groundwork to make future work easier. And that's actually one of the biggest reasons behind my tendency to check the rulebook (harp on certain conventions): Because most of the ones I try to stick to have pretty solid backing behind them. But I don't think anyone's ever really pushed back on anything that I've made at least a half-decent case for, here, on technical grounds. But I also think my verbosity for sake of covering as many bases at once that I can comes off as though I feel much more strongly than I really do about a lot of things, because I've just found throughout life that things move along a lot quicker if I don't try to explain the 5 mile long train I'm on and just talk about how this particular car can be pimped out for now and then get to the rest later. 😅 Although my #1 preference is a small and passionate group of people with compatible personalities but at least different approaches/strategies for tackling whatever they're presented with, whiteboards all around the room, a whole package of different colored markers with everyone always having a marker in-hand, and everyone giving their thoughts openly and without fear of judgment, resulting in coming up with something awesome (even if awesome in a given context means remarkably simple/elegant) that everyone already groks from the start because it's got a little bit of each of us in every facet. 🙂 I had the perfect team for that, at one job, and loved it so much before [insert predictable story about ham-handed corporate organizational restructuring by new execs wanting to make a mark here] happened. Haven't quite had the same level of it since then, but there are moments. And that's where it becomes a secant rather than a tangent: It's why, even though, sure, I could just do the work and then modify based on any feedback anyone feels strongly enough to give, I instead prefer to ask for input on something fundamental like this one that I already have a half-formed opinion on but want the other perspective on, if there is one - even if it's just spitballing. I like spitballing. Got a straw right here in fact.
Footnotes |
|
Oh whoops. Looking for a silly GIF made me forget the last thing I wanted to mention that was also a motivation behind specifically asking you the question, directly: Regardless of the frequency or force with which someone puts their foot down in a project, I'm always cognizant about the artistic/creative element of what we as software engineers do, so there's an element of deference in the question, insofar as the shape of my work will morph to make something with at least a small metaphorical signature of the project principal. Heck, that could even just be liking "DuckBill" over "SchoolBus" for the name of a bright yellow-orange color or something. 🤷♂️ |
|
I love all of the 👆. I even read it all. Twice. I'd like to see a double-buffer that could be extended to 3 design. The things I think / have thought about related to this: TG needs to provide an abstraction layer above terminals.
Some proposed Tenets (in addition to those here: https://github.com/gui-cs/Terminal.Gui/blob/40056a6f2c410996537946a69e0f1b4a43a044db/CONTRIBUTING.md)
What others might you suggest? Related issues where I've written thots:
Then there's the whole discussion of by whom and when Adornments should be drawn. I'm PRETTY SURE I can make the current design work and fix this: But, if I'm wrong, to get the amazing ability to have lines/text "auto join" even when Views are independent, we may need a compositor mechanism that's not I will know for sure when this is tackled: Great stuff @dodexahedron . Footnotes |
|
Oh yea: We need to recognize that by tackling we are effectively building a new Right now, the code in the |
|
Whoops. Apparently I didn't click the comment button hard enough this morning when I wrote this up. 😅 But I'll add a TL;DR, which is: 🥳 TL version follows: Sounds like we were on the same page and didn't even realize it. 😆
Exactly what I was initially thinking. If it really isn't much more to get triple, when work is being done for that, I probably will just go ahead and go all the way from the start. Otherwise, yeah - I'd go for exactly that idea.
1000000% And that's the most significant underlying goal of this change series, in fact, though not explicitly stated in case I wanted to break out into another change set instead. Probably will, anyway, just for organizational purposes, but the work being done right now is definitely aimed at violently shoving some things in that direction. Fortunately for us, too, .net itself already does provide at least some abstractions, which are the most visible parts of what I'm adopting in this work. And the facilities for that stuff that are available in .net actively continue to improve in various ways - some subtle and some pretty important. So that's cool for us, but just a side-note here. But meshes with your next bullet, somewhat:
Absolutely. Even just look at the configuration screens in PuTTY for an abbreviated history and collection of various common quirks. 😅 Or at least I assume it still has that screen - ever since windows got openssh, I haven't used putty ever again. And even within the same major.minor version of the same program, there can be differences that matter to us because of what TG is. Sucks for us, but that's kind of the point of a library like this. Yeah, we give you standard TUI controls, but we also let you do so the same way across at least a reasonable range of environments by doing that for you.
It's almost funny to lump those three together in so terse a way in one little bullet, with how big each one is, even all on its own. 😅
Yep. Although at least some of that we get to rely on the lower layers for (or should, to an extent, anyway). SSH, for example, is already over TCP, so order we receive things in isn't our responsibility (or place) to interpret in any other order than that in which it was delivered to us, since TCP won't be handing us anything not in the same order it was transmitted. But yes - any session with a high BDP has high potential to deliver a suboptimal experience to the user. To that end, one of the most significant things we can do is one that's already in progress, which is adopting terminal control sequences wherever we can, to cut down on the impact of high BDP. Later on, we could/should also take a pass over things to determine if there are existing operations that can be made lighter-weight by using more of those, as well. But another broad thing about abstraction overall, applicable to all of the preceding points, is that the current state of the driver infrastructure/implementation is not extensible outside of TG itself. There are required pieces that are exposed in the public API surface, but impossible to implement due to those pieces having tight coupling with internal types. That's something I'm fixing during this work, too and is a part of things I've been planning for a long time and have mentioned a few times this past year. That being interfaces. Once I have gotten a little more done, I'm going to extract interfaces for everything in the driver stack and make it all use those interfaces. Then we can even make the actual implementations themselves internal if we really wanted to (probably shouldn't of course), because it will force de-coupling some things and, more importantly, will ensure that anything that is dependent on anything else in that stack is visible and has a guaranteed API contract. I happen to have actually already done a bit of that on a separate branch, early on in this work, just to see how much work it would take. It's actually not that bad, now, thanks to other work that's been done along the way for V2 and what's being done related to this change series. The reason I paused that work was actually because that is what showed me a lot of what else needed to be done anyway, and I would have ended up deleting a bunch of work that the rest of this work will obviate. For the most part, though, outside of places where the coupling needs to be fixed, consumption of the interfaces when I was doing that were almost universally as simple as changing
Ha yeah. Always fun. Another good place to ensure that we not only provide a solid base functionality, but also make extensibility possible without requiring recompiling TG itself.
Those are a solid starting point. Maybe adding something that meshes with the last point above those, about respecting the user and not forcing things upon them. Another that I've had to mention a couple times that I think deserves its own bullet: "Terminal.Gui is not the application." It is a library. It is just one frankly insignificant component consumed by an application, and needs to get out of the way for any and all purposes not directly related to user interaction. Thus, any assumptions made absolutely critically must be passed through that filter, or else we are putting the consumer in a frustrating box they didn't need to be put in. So, thread/concurrency-safety, lifecycles of anything that isn't strictly gen 0, resource consumption (of all types), accessibility, type safety, exception throwing and handling, and even simple matters of formality (at least in the public API surface) all should be given their due respect. For the most part, they usually are, or are at least thought about, but I think it is worth reminding any contributor that TG is not the application and needs to act that way.
Agreed at a high/conceptual level. Which actual type they shouldn't directly be using may not yet be fully fixed (or may just end up being a naming thing). But conceptually, right: No, they should not be directly using any of the low-level internal implementations in TG - only a public interface that lays out the required minimal contract to use them. Not allowing any derivation via a mechanism like that would be needlessly draconian, though, for no benefit to us. But I don't think that's your intent anyway. Heh yep. This one I guess finally tackles that one. I think the desired result can be achieved by simply utilizing the standardized logging framework abstractions provided by Microsoft.Extensions.Logging, which are pretty much ubiquitous elsewhere and are completely agnostic of framework/implementation. Just add statements to log the low-level stuff at the appropriate level to an appropriate target and then, if/when needed, enable that in your config and it'll "just work." Not to engage in post-mortem pugilism with this equine critter too much, but we should do that, regardless. Yep. Ideally this change series will finally make that reasonably possible, or at least make it not impossible, so it can be addressed afterward. I've been eyeing Cell a lot while working on this. I've got a bunch of different thoughts stewing on that topic. Will definitely have to come back to that once this is done. Yep. The main pain point I think we'll have to deal with is that the level of support for VTS varies a fair bit from version to version and from host to host, on the same system. We just need to be sure we account for that in some way, even if it's as simple as sticking to the lowest common denominator for sequences we use, for the easy way out. I'm pretty tired after the rabbit holes I went down (unrelated to TG) before coming back to this, so I won't give direct commentary on the rest of these bullets, since I have not yet looked them over, but the bullets themselves look like 👍 to me across the board:
And this work may also impact that. If all goes according to plan, life should be easier. 🙂
Addition not in the original: Ha. I read the blog post today. The last line of the last commenter's comment reads "Basically, doing it right ends up rewarding you in unexpected ways, and doing it wrong ends up biting you on the ass in unexpected ways." I say scratch the "un" from the second to last word. 😆 |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Whoops. I meant for that to go in the PR. Gonna take care of that... |
|
(@tig you in particular may find this interesting... Or want the following few minutes back... One of those, probably...) You know... As I was working on the beginnings of a platform abstraction layer, today, I thought, "OK... There's no way this hasn't at least been partially done already, or else how can System.Console work at all, across platforms?" So I went diving through the dotnet source on github, at the v8.0.8 tag. There's a wealth of stuff in there that is exactly what we need/want for quite a few scenarios (or pretty close), some of which I was beginning to write almost verbatim re-implementations of, but won't have to now that I've found them. ...But they're all internal types, in .net 8, with a small number marked public in .net 9. But hey - doesn't mean I can't use it now (yay open source with same license)!1 There's also plenty in there that, while not necessarily directly useful, provides excellent templates for or at least inspiration for what I do need. I don't need the full implementations of a lot of it - just some types, marshallers, and a base class or two to cut the code I have to write for the custom marshalling down by... a lot...2 While I'm mostly talking about marshalling and PInvoke, specifically, here, it's not limited to that. But that's a big piece of the puzzle, and I'm eager to get back to work on it, so I'm holding myself to a 20-page limit on this comment.3 😛 If I do end up taking any actual code directly from the runtime, I'll keep them API-compatible just in case we are granted any gifts of more of them becoming public in the future. But... Shifting gears back to the topic of abstractions... Platform interop, especially if properly abstracted, is an independent, separable component of the library/framework, and there are tangible benefits to separating it to its own component library, as well as potential negative impacts if that isn't done. Why? Why is it being an assembly-level attribute a problem, though (thus prompting the need for separation)? A biggie for those two (so...PInvoke), specifically, is that All non-source-generated marshalling is off, so you have two options:
Aside from the PInvoke impact, it also alters how some delegates are handled internally, and imposes strict limitations on the types used for the extern methods, because they MUST comply with the rules/list here. The flip-side of that is that those restrictions make the rules for what can be used quite simple. As above, things either need to be fully blittable and identical to their native representation, OR there must be a marshaller defined that the source generator can make use of. In any case, I'm pretty sure it would break some of the existing pre-cached delegate implementation inherited from V1 (which is already on the chopping block anyway, so no loss there), but I'm really not interested in trying to find out what, where, and why things are broken by that, since it can all be avoided by just putting interop into its own assembly and doing things the way the native interop docs describe. Then, interop code can be under the auspices of the Another really nice benefit to that is that, if I write it as a base abstraction layer6, with the platform-specific implementations inheriting from/implementing that API, it becomes naturally pluggable, and consumers can leave out the platform-specific assemblies they don't want or need, if size or attack surface or anything else related to dead code is a concern. I wrote up a bunch of POCs today in a toy app through which I pass darn near everything I write of this nature7, to feel out all the concepts involved in all this and more, including custom marshallers (which will mostly or possibly completely eliminate my explicit hand-written PInvoke code layer), safer memory management, buffer pooling, and various console and platform abstraction concepts to make our and consumers' lives easier. Lots of good results, lots of good learning, and some interesting exposition of issues that need to be addressed even if this whole exercise were to be scrapped and the existing implementation used for the time being - some of which are security-critical to a rather frightening degree8, but easy to fix and getting fixed, no matter how the rest of this work evolves. Alright, I wanna get back to work. This is plenty of stuff to chew on, anyway. ...Hey! I kept it under the character limit!9 Footnotes
|
|
Very interesting. 🤨 I've not looked at the .Console code. Glad you have. I'd be curious to know what's going on relative to this: |
It's all over the place throughout coreclr, with pieces in at least 6 different libraries . In fact, a fair amount of the platform interop code is actually separated into specific DLLs for various reasons including that those have DisableRuntimeMarshalling specified, but most of the rest of coreclr does not. A lot of that used to be in C, C++, or C++/CLR, but they've really been making an effort to move as much of everything everywhere into pure C# as they can in the past several years, since pinning is expensive, among other reasons.
Yeah there are definitely some fun bits. It's kinda interesting/odd to me that all the PInvoke code, including types, functions, constants, and marshalling, where necessary, sometimes even with XmlDoc comments, is already in .net in classes named for the library they're in, such as As for Console, the underlying platform code for parts of it (which is one of many places coreclr has a PAL) of course uses quite different APIs on each platform, particularly around HCI like mouse input, but then the same (or at least similar) APIs for other bits where possible. Ironically, on Unixy platforms, one of the methods of a class called StdInReader, which is simply an internal sealed class derived from TextReader (which is what Console.In is), has this to say on its implementation of ReadKey:
And here's the code of StdInReader.ReadKey as of the v8.0.8 tag: public ConsoleKeyInfo ReadKey(out bool previouslyProcessed)
{
if (_availableKeys.Count > 0)
{
previouslyProcessed = true;
return _availableKeys.Pop();
}
previouslyProcessed = false;
return ReadKey();
}
private unsafe ConsoleKeyInfo ReadKey()
{
Debug.Assert(_availableKeys.Count == 0);
Interop.Sys.InitializeConsoleBeforeRead();
try
{
if (IsUnprocessedBufferEmpty())
{
// Read in bytes
byte* bufPtr = stackalloc byte[BytesToBeRead];
int result = ReadStdin(bufPtr, BytesToBeRead);
if (result > 0)
{
// Append them
AppendExtraBuffer(new ReadOnlySpan<byte>(bufPtr, result));
}
else
{
// Could be empty if EOL entered on its own. Pick one of the EOL characters we have,
// or just use 0 if none are available.
return new ConsoleKeyInfo((char)
(ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter :
ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character :
ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter :
0),
default(ConsoleKey), false, false, false);
}
}
return KeyParser.Parse(_unprocessedBufferToBeRead, ConsolePal.TerminalFormatStringsInstance, ConsolePal.s_posixDisableValue, ConsolePal.s_veraseCharacter, ref _startIndex, _endIndex);
}
finally
{
Interop.Sys.UninitializeConsoleAfterRead();
}
}In the ConsolePal.Windows.cs version, ReadKey is handled at that layer and doesn't wrap the stream in a bespoke class, because it just immediately runs to the Win32 PInvokes, rather than using streams, since Windows doesn't include anything but keys when you read the handle as a normal file/stream, requiring you to use the ReadConsoleInput function to get the other stuff.: public static ConsoleKeyInfo ReadKey(bool intercept)
{
Interop.INPUT_RECORD ir;
bool r;
lock (s_readKeySyncObject)
{
if (_cachedInputRecord.EventType == Interop.KEY_EVENT)
{
// We had a previous keystroke with repeated characters.
ir = _cachedInputRecord;
if (_cachedInputRecord.keyEvent.wRepeatCount == 0)
_cachedInputRecord.EventType = 0;
else
{
_cachedInputRecord.keyEvent.wRepeatCount--;
}
// We will return one key from this method, so we decrement the
// repeatCount here, leaving the cachedInputRecord in the "queue".
}
else
{ // We did NOT have a previous keystroke with repeated characters:
while (true)
{
r = Interop.Kernel32.ReadConsoleInput(InputHandle, out ir, 1, out int numEventsRead);
if (!r)
{
// This will fail when stdin is redirected from a file or pipe.
// We could theoretically call Console.Read here, but I
// think we might do some things incorrectly then.
throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
}
if (numEventsRead == 0)
{
// This can happen when there are multiple console-attached
// processes waiting for input, and another one is terminated
// while we are waiting for input.
//
// (This is "almost certainly" a bug, but behavior has been
// this way for a long time, so we should handle it:
// https://github.com/microsoft/terminal/issues/15859)
//
// (It's a rare case to have multiple console-attached
// processes waiting for input, but it can happen sometimes,
// such as when ctrl+c'ing a build process that is spawning
// tons of child processes--sometimes, due to the order in
// which processes exit, a managed shell process (like pwsh)
// might get back to the prompt and start trying to read input
// while there are still child processes getting cleaned up.)
//
// In this case, we just need to retry the read.
continue;
}
if (!IsReadKeyEvent(ref ir))
{
continue;
}
if (ir.keyEvent.wRepeatCount > 1)
{
ir.keyEvent.wRepeatCount--;
_cachedInputRecord = ir;
}
break;
}
} // we did NOT have a previous keystroke with repeated characters.
}
ControlKeyState state = (ControlKeyState)ir.keyEvent.dwControlKeyState;
bool shift = (state & ControlKeyState.ShiftPressed) != 0;
bool alt = (state & (ControlKeyState.LeftAltPressed | ControlKeyState.RightAltPressed)) != 0;
bool control = (state & (ControlKeyState.LeftCtrlPressed | ControlKeyState.RightCtrlPressed)) != 0;
ConsoleKeyInfo info = new ConsoleKeyInfo(ir.keyEvent.uChar, (ConsoleKey)ir.keyEvent.wVirtualKeyCode, shift, alt, control);
if (!intercept)
Console.Write(ir.keyEvent.uChar);
return info;
}For the record, one of the comments in that code that says they might be able to use Console.Read but it might break certain things is something I happened to confirm for myself, by chance, when I was trying to do exactly that in a PoC yesterday. You end up having to call additional kernel32 functions first to make it work like one would expect if you do that, and it has high potential to be fragile if a lot of care is not taken around various contingencies. So, for windows, it's best to just KISS and use ReadConsoleInput to drain the input buffer, as that code does, when all HCI input is wanted. And I have a suspicion that this Windows Terminal issue that another comment in the code mentions (microsoft/terminal#15859) may have something to do with the odd behavior around things you raised your issue over in the dotnet/runtime repo for. At the end of the day, the fundamentally different way that Unix and Windows treat the IO streams for TTYs1 creates the precondition for the different results, and historical baggage plus trying to help perhaps a bit too much in Windows. kernel32 involves itself in the interpretations of the bytes, rather than just being a dumb pipe,2 making VTS the platform's responsibility, rather than the terminal emulator's responsibility, which makes things more complicated, at the lower level. Or at least that part stands out to me as a kink in the process, anyway. Ironically, unix not special-casing it ends up making life easier since it's your terminal emulator's responsibility to interpret the bytes you give to its stdout stream, and the OS just hands them to it and gets out of the way for the most part. Buuuut then you want mouse input too, and...well... fun! As I've been deep-diving into it all, I've more than once found myself wondering if allocating in-memory streams/buffers over a screen buffer equivalent3, allocating our own hosted PTY4, and just proxying the IO to the console the application was handed at launch might be the fix needed to make it all work consistently and more simply. But I have more research and prototyping to do to see if that's viable or even actually solves what I'm hypothesizing it would solve. But it sure would be a nice help in making a unified platform-agnostic abstraction if we could deal with all platforms using a PTY. That also makes network scenarios easier, as well, since a PTY has no requirement to be attached to the application hosting either of its streams. That, incidentally, also creates cool possibilities of things like a network driver that gives you a direct remote console to a running application on another machine, like a text-mode X sort of situation, not needing ConHost.exe or bash or whatever terminal emulator to even be in the picture.5 Another somewhat useful resource, but only for insight (due to incompatible LGPL-2.1 licensing) has been ReactOS, since that attempts to be a clean-room API- and ABI-compatible implementation of the Win32 APIs, from user space down to kernel space, including driver compatibility. Just sucks that LGPL-2.1 is viral and requires the entirety of a combined work using it to then also be under that license.6 😤 <--(I had a lot of trouble trying to get github to let me use that emoji) Anyway, back to platform abstractions... It's awesome that Windows Terminal is what it is, because work on that is forcing some attention to be paid to System.Console and related code/functionality that we're getting direct and indirect benefits from, through the changes/enhancements being made in coreclr to support it, with apparent respect and care being given to cross-platform implications of it all. I've been watching progress at the WT repo and using it as a reference resource, as well, since obviously there's a teensy bit of relevance, there, for what we're doing. Footnotes
|
dodexahedron
changed the title
|
Another thought about something that just kinda amuses me in a way is that the code quoted above does some work that we then undo and then redo in various places. In particular, that's around modifier keys, which you can see the handling of at the bottom of the second code block (the windows version of ReadKey). Not sure what, if any, relevance that will have as work continues, but it did stand out to me. If anything, maybe it'll help inform how the abstractions take shape. 🤷♂️ |
|
Quick update: Have been continuing to research and prove things out in simple isolated toy apps for various concepts. When I re-branch for this, I'll have some nice new tools in the toolbelt for simplifying and improving the drivers and abstractions. But today and probably tomorrow as well, I have some other pressing work that I need to get done not related to TG. |
|
OMG. I can log into github again! I've been back for like a week after the stuff i mentioned was finally over/completed, but was having trouble here... Sorry. Mostly my fault, as it turns out. But it's fixed in a 3x failsafe way now. 😅 Anyway... I'm back and will be getting myself back up to speed over the next few days, as far as work that's happened in the last few weeks, so I can get back to work on this. Haven't looked over anything in my gh inbox yet, though. Just had this up toward the top and wanted to make contact so y'all know I'm alive and didn't ditch ya or something. 😅 |
|
Missed you! |
|
@dodexahedron I have started simplifying NetDriver as part of my work adding AnsiResponseParser (AnsiResponseParser changes are #3791 , net driver changes are https://github.com/tznind/gui.cs/pull/172/files) Hopefully that won't cause too much conflict with your work here. I am basically
The diff (to the main ansi branch) can be seen here: https://github.com/tznind/gui.cs/pull/172/files In fact we can probably even use something like: |
|
Since I backed it out to do something even cleaner in the first place than what I had previously submitted a PR for, there's no conflict, right at the moment. But, I would suggest/recommend that you use FileStreams to the pty anyway. A queue ends up not really being necessary, though you certainly can use that on top of things. FileStream is a fully cross-platform means of dealing with the terminal and is how libc does it and ultimately how win32 does it, as well (but windows doesn't implicitly separate stderr and stdout unless you tell it to, so they are different handles to the same stream by default). Doing that, you have direct access to the raw buffer, avoiding all the marshaling overhead and complexity involved with a lot of the pinvokes that are currently in place. Some are still necessary, for certain bits, particularly on Windows, but they all get a common base implementation, which is so lovely. :) I'll just start up again on not-NetDriver when I get back to it. |
|
It's kinda interesting. It's a FileStream, but it feels and behaves almost exactly like a NetworkStream on top of a UDP Socket or other unordered pair of unidirectional channels. It's forward-only/non-seekable, for both input and output, and you'll receive things in the order the OS dispatched them to you, which is what you'll get any other way, but with less junk in between and fewer platform-specific idiosyncrasies to deal with. It's one place where Windows really shows its original roots and inspiration, because the console is really two character device files that you write to and read from (called $CONIN and $CONOUT), just like Unix, where literally everything is a file. WinNT and VMS share interesting history there. There's a third virtual character device file that is r/w, called just CON, and that was a handy thing to know back in the DOS days if you didn't have a text editor available. You could Fun stuff. Oh yeah - and you can still do all of that - even the printer output. Just pipe or copy to Footnotes
|
Filing an actual issue now that I've gotten to work on things that have been talked about here and there and some discretionary/opportunistic bits along the way.
This one starts with the console drivers.1
I'm nearly done with what I'm slapping a round 1 label on and will have a PR ready later today, hopefully.
Things that are addressed so far:
Typevalues.MemberData)I've done it as a ton of mostly smaller commits, as usual, to make it easier to follow along through the commit graph. However, one of the biggest changes I made at the start is actually what prompted some of the additional work that got done in this round, and hasn't been committed to the branch yet, so the current head of https://github.com/dodexahedron/Terminal.Gui/tree/v2_d12_some_driver_cleanup_1 will not compile at the moment, though my local copy (which of course DOES have those changes) compiles and passes all tests. I just need to finish the work on it before I can commit and push that part.
PR will come when I push the commits with that work finished.
There are also a few extra bits of work I want to do before round 1 is done, beyond that, but those will come after a draft PR is submitted.
Footnotes
No specific reason about the code or code quality itself for picking that as a starting point for this work. It was based on the fact that they're alphabetically early in the project folder tree, they're some of the first code that runs, and the first warnings in the list when I turned analysis up a notch locally brought me to one of those files. ↩
The text was updated successfully, but these errors were encountered: