-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
One process to rule them all #14843
Merged
Merged
One process to rule them all #14843
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing AppLogic that I hate so I'm gonna start over
This reverts commit a5255ba.
We'll need this for #5000, for ainulindale. This refactoring will be annoying enough as it is so we may as well do it as a first, separate PR.
…ns though, so it immediately exits
…ndale # Conflicts: # src/cascadia/WindowsTerminal/AppHost.h
…XAML island is started. I think this can work in the parent at least
…It launches and crashes immediately. We'll keep shuffling code.
At this point, I determined that I would need to make some big changes to AppHost and decided that it was time to commit before moving on.
…pressive It exits itself after 30s, but hey it worked
(cherry picked from commit 57d1dd4)
Doesn't work with multiple windows open, but doesn't do _nothing_ (cherry picked from commit 427a4a5)
…ut this doesn't fix the crash (cherry picked from commit 700aadc)
TerminalPage is the thing that ends up expanding iterable Command. It does this largely with copies - it makes a new map, a new vector, copies the Commands over, and does the work there before setting up the cmdpal. Except, it's not making a copy of the Commands, it's making a copy of the vector, with winrt objects all pointing at the Command objects that are ultimately owned by CascadiaSettings. This doesn't matter if there's only one TerminalPage - we'll only ever do that once. If there's many, on different threads, then one tpage will end up expanding the subcommands of one Command while another tpage is ALSO iterating on those subcommands. Hence why I'm getting `hresult_changed_state`s (cherry picked from commit 2122eec)
36 TODOs
…ispatcher. 35 TODOs left
32 TODOs
30 TODOs left
This was referenced Mar 13, 2023
lhecker
requested changes
Mar 16, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking because of the reference cycle issue.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
The original author of the issue/PR needs to come back and respond to something
label
Mar 16, 2023
Each page was registering as a handoff target, so basically we'd start the server then yeet the connection back to the first window and presto, you'd have a dead window and a connection on the wrong thread and everything was awful. Instead, only register as the handoff listener when we've actually said we want to be a handoff listener.
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
The original author of the issue/PR needs to come back and respond to something
label
Mar 17, 2023
lhecker
approved these changes
Mar 17, 2023
…lindale # Conflicts: # src/cascadia/TerminalApp/AppLogic.cpp # src/cascadia/TerminalApp/AppLogic.h # src/cascadia/TerminalApp/AppLogic.idl # src/cascadia/TerminalApp/TerminalPage.h # src/cascadia/TerminalApp/TerminalWindow.cpp # src/cascadia/TerminalApp/TerminalWindow.h # src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp # src/cascadia/TerminalSettingsModel/GlobalAppSettings.h # src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl # src/cascadia/WindowsTerminal/AppHost.cpp # src/cascadia/WindowsTerminal/AppHost.h
DHowett
approved these changes
Mar 17, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's do it!
DHowett
pushed a commit
that referenced
this pull request
Mar 22, 2023
## Summary _Thus we come to the introduction of a new servant, the `ContentManager`, a singular entity that serves at the behest of the `emperor`. It is its charge to keep track of all `TermControl` instances created by the windows, for each window must seek its blessing before calling forth such an instance._ _With the aid of the `ContentManager`, the `TermControl` shall now be traced by the hand of fate through the use of unique identifying marks, known as `GUID`s. Yet, its purpose remains yet unknown, for it is merely a waypoint upon the journey yet to come._ _This act of bridging also brings a change to the handling of events within the `TermControl`. This change shall see the addition of a revoker, similar to the manner in which the `AppHost` hath employed it, to the `TermControl`. Additionally, there is a new layer of indirection between the `ControlCore` and the `App` layer, making ready for the day when the `TermControl` may be repositioned and re-parented with ease._ _Consider this but a trivial act, a mere shadow of things yet to come, for its impact shall be felt but briefly, like the passing of a gentle breeze._ Related to #5000 Related to #1256 # Detailed description This PR is another small bridge PR between the big work in #14843, and the PR that will enable panes to move between windows. This introduces a new class, called `ContentManager`. This is a global singleton object, owned by the emperor. Whenever a window wants to instantiate a new `TermControl`, it must ask the ContentManager to give it one. This allows the ContentManager to track each "content" by GUID. That's it. We don't do anything with them in this PR by itself, we just track them. This also includes a small change to the way TermControl events are handled. It adds an `AppHost`-like revoker struct, and weak_ref's all the handlers. We also add a layer of indirection between the ControlCore's raising of events and the App layer's handling. This will make reparenting content easier in the future. This is a pretty trivial change which shouldn't have any major side effects. Consider it exposition of the things to come. It's intentionally small to try and keep the reviews more managable.
DHowett
pushed a commit
that referenced
this pull request
May 8, 2023
DHowett
pushed a commit
that referenced
this pull request
May 26, 2023
…5451) This is a resurrection of #5629. As it so happens, this crash-on-exit was _not_ specific to my laptop. It's a bug in the XAML platform somewhere, only on Windows 10. In #14843, we moved this leak into `becomeMonarch`. Turns out, we don't just need this leak for the monarch process, but for all of them. It's not a real "leak", because ultimately, our `App` lives for the entire lifetime of our process, and then gets cleaned up when we do. But `dtor`ing the `App` - that's apparently a no-no. Was originally in #15424, but I'm pulling it out for a super-hotfix release. Closes #15410 MSFT:35761869 looks like it was closed as no repro many moons ago. This should close out our hits there (firmly **40% of the crashes we've gotten on 1.18**)
DHowett
pushed a commit
that referenced
this pull request
May 26, 2023
…5451) This is a resurrection of #5629. As it so happens, this crash-on-exit was _not_ specific to my laptop. It's a bug in the XAML platform somewhere, only on Windows 10. In #14843, we moved this leak into `becomeMonarch`. Turns out, we don't just need this leak for the monarch process, but for all of them. It's not a real "leak", because ultimately, our `App` lives for the entire lifetime of our process, and then gets cleaned up when we do. But `dtor`ing the `App` - that's apparently a no-no. Was originally in #15424, but I'm pulling it out for a super-hotfix release. Closes #15410 MSFT:35761869 looks like it was closed as no repro many moons ago. This should close out our hits there (firmly **40% of the crashes we've gotten on 1.18**) (cherry picked from commit aa8ed8c) Service-Card-Id: 89332890 Service-Version: 1.18
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Area-Remoting
Communication layer between windows. Often for windowing behavior, quake mode, etc.
Area-Windowing
Window frame, quake mode, tearout
Issue-Task
It's a feature request, but it doesn't really need a major design.
Product-Terminal
The new Windows Terminal.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
followed by: #14851
Map
AppLogic
into "App logic" and "Window logic" #14825Summary
In the days of old, the windows were sundered, each with its own process, like the scattered stars in the sky. But a new age hath dawned, for all windows now reside within a single process, like the bright gems of a single crown.
And lo, there came the
WindowEmperor
, a new lord to rule over the global state, wielding the power of hotkeys and the beacon of the notification icon. TheWindowManager
was cast aside, no longer needed to seek out other processes or determine theMonarch
.Should the
WindowEmperor
determine that a new window shall be raised, it shall set forth a new thread, born from the ether, to govern this new realm. On the main thread shall reside a message loop, charged with the weighty task of preserving the global state, guarded by hotkeys and the beacon of the notification icon.Each window doth live on its own thread, each guarded by the new
WindowThread
, a knightly champion to hold theTerminalWindow
,AppHost
, andIslandWindow
in its grasp. And so the windows shall run free, no longer burdened by their former ways.All windows are now in a single process, rather than in one process per window. We'll add a new
WindowEmperor
class to manage global state such as hotkeys and the notification icon. TheWindowManager
has been streamlined and no longer needs to connect to other processes or determine a newMonarch
. Each window will run on its own thread, using the newWindowThread
class to encapsulate the thread and manage theTerminalWindow
,AppHost
, andIslandWindow
.Windows Terminal Process Model 3.0
Everything is now one process. All the windows for a single Terminal instance live in a singular Terminal process. When a new terminal process launches, it will still attempt to communicate with an existing one. If it finds one, it'll pass the commandline to that process and exit. Otherwise, it'll become the "monarch" and create a new window.
We'll introduce a new abstraction here, called the
WindowEmperor
.Monarch
&Peasant
will still remain, for facilitating cross-window communication. The Emperor will allow us to have a single dedicated class for all global state, and will now always represent the "monarch" (rather than our previously established non-deterministic monarchy to elevate a random peasant to the role of monarch). We still need to do a very minimal amount of x-proc calls. Namely, one right on startup, to see if anotherTerminal.exe
was already running. If we find one, then we toss our commandline at it and bail. If we don't, then we need toCoRegister
the Monarch still, to prepare for subsequent launches to send commands to us.WindowManager
takes the most changes here. It had a ton of logic to redundantly attempt to connect to other monarchs of other processes, or elect a new one. It doesn't need to do any of that anymore, which is a pretty dramatic change to that class.This creates the opportunity to move some lifetime management around. We've played silly games in the past trying to have individual windows determine if they're the singular monarch for global state.
IslandWindow
s no longer need to track things like global hotkeys or the notification icon. The emperor can do that - there's only ever one emperor. It can also own a singular copy of the settings model, and hand out references to each other thread.Each window lives on separate threads. We'll need to separately initialize XAML islands for each thread. This is totally fine, and actually supported these days. We'll use a new class called
WindowThread
to encapsulate one of these threads. It'll be responsible for owning theTerminalWindow
,AppHost
andIslandWindow
for a single thread.This introduces new classes of bugs we'll need to worry about. It's now easier than ever to have "wrong thread" bugs when interacting with any XAML object from another thread. A good case in point - we used to stash a
static
Brush
inPane
, for the color of the borders. We can't do that anymore! The first window will end up stashing a brush from its thread. So now when a second window starts, the app explodes, because the panes of that window try to draw their borders using a brush from the wrong thread.Another fun change: The keybinding labels of the command palette.
TerminalPage
is the thing that ends up expanding iterableCommand
s. It does this largely with copies - it makes a newmap
, a newvector
, copies theCommand
s over, and does the work there before setting up the cmdpal.Except, it's not making a copy of the
Command
s, it's making a copy of thevector
, with winrt objects all pointing at theCommand
objects that are ultimately owned byCascadiaSettings
.This doesn't matter if there's only one
TerminalPage
- we'll only ever do that once. However, now there are many Pages, on different threads. That causes oneTerminalPage
to end up expanding the subcommands of aCommand
while anotherTerminalPage
is ALSO iterating on those subcommands.Emperor message window: The Emperor will have its own HWND, that's entirely unrelated to any terminal window. This window is a
HWND_MESSAGE
window, which specifically cannot be visible, but is useful for getting messages. We'll use that to handle the notification icon and global hotkeys. This alleviates the need for the IslandWindow to raise events for the tray icon up to the AppHost to handle them. Less plumbing=more good.Class ownership diagram
pretend that I know UML for a second:
Monarch
. One for the Terminal process.Peasant
s, one per window.Monarch
is no longer associated with a window. It's associated with theEmperor
, who maintains all the Terminal windows (but is not associated with any particular window)Remoting
dll is concerned, it doesn't care if monarchs and peasants are associated with windows or not. Prior to this PR, yes, the Monarch was in fact associated with a specific window (which was also associated with a window). Now, the monarch is associated with the Emperor, who isn't technically any of the windows.Emperor
owns theApp
(and by extension, the singleAppLogic
instance).WindowThread
object.AppHost
, oneIslandWindow
, oneTerminalWindow
&TerminalPage
per window.AppLogic
hands out references to its settings to eachTerminalWindow
as they're created.Isolated Mode
This was a bit of a tiny brainstorm Dustin and I discussed. This is a new setting introduced as an escape watch from the "one process to rule them all" model. Technically, the Terminal already did something like this if it couldn't find a
Monarch
, though, we were never really sure if that hit. This just adds a setting to manually enable this mode.In isolated mode, we always instantiate a Monarch instance locally, without attempting to use the
CoRegister
-ed one, and we never register one. This prevents the Terminal from talking with other windows.wt -w foo
) won't workA diagram about settings
This helps explain how settings changes get propagated
TODO!s