Refactor ConsoleProcessList #14421

lhecker · 2022-11-21T22:05:19Z

This commit is just a slight refactor of ConsoleProcessList which I've noticed
was in a poor shape. It replaces iterators with for-range loops, etc.

Additionally this fixes a bug in SetConsoleWindowOwner, where it used to
default to the newest client process instead of the oldest.

Finally, it changes the process container type from a doubly linked list
over to a simple array/vector, because using linked lists for heap allocated
elements felt quite a bit silly. To preserve the previous behavior of
GetProcessList, it simply iterates through the vector backwards.

Validation Steps Performed

All unit/feature tests pass ✅
Launching a TUI application inside pwsh inside cmd
and exiting kills all 3 applications ✅

lhecker · 2022-11-21T23:34:44Z

src/interactivity/win32/windowio.cpp

@@ -78,7 +78,7 @@ VOID SetConsoleWindowOwner(const HWND hwnd, _Inout_opt_ ConsoleProcessHandle* pP
    else
    {
        // Find a process to own the console window. If there are none then let's use conhost's.
-        pProcessData = gci.ProcessHandleList.GetFirstProcess();
+        pProcessData = gci.ProcessHandleList.GetOldestProcess();


This is a behavior change: GetFirstProcess() used to return the newest process, but I think this was wrong... conhost's PID was the last (= oldest) entry if anything. In fact even the SetConsoleWindowOwner documentation says:

If ProcessData is nullptr that means the root process has exited so we need to find any old process to be the owner.

So I've chosen to rename this function to better convey its meaning and fix this bug.

any old has an idiomatic meaning, though.

Is the comment "If there are none then let's use conhost's." still accurate, or does this now use conhost's process even if others exist?

any old has an idiomatic meaning, though.

Ah good point.

Is the comment "If there are none then let's use conhost's." still accurate, or does this now use conhost's process even if others exist?

I'm not sure it ever used to be accurate. conhost itself isn't part of the process lists - console clients are (i.e. shells and other console subsystem applications). I wouldn't know how conhost could be its own client.
In reality this code path is only invoked via Window::SetOwner when the "root process" exits (the first process that is launched - usually your shell, like cmd.exe for instance), but there are still other clients attached. This is for instance reproducible if you run pwsh.exe inside cmd.exe and close the window. Both processes will get get a close event simultaneously, but PowerShell is slow and so it'll take longer to exit then cmd.exe. Once cmd.exe is gone, conhost will need a new "root process" and in this case that would be pwsh.exe (until it also exits a hundred or so milliseconds later).

So basically, the comment is at least generally wrong/inaccurate, but I don't want to touch it, because it might prove to be a useful nugget of information if looked up via git blame in the future. The old code used to use the newest process as the new root process and that seems like a mistake to me. If anything it should be the oldest one as far as I can see.

lhecker · 2022-11-21T23:35:11Z

src/server/ProcessList.cpp

@@ -30,51 +27,27 @@ using namespace Microsoft::Console::Interactivity;
 [[nodiscard]] HRESULT ConsoleProcessList::AllocProcessData(const DWORD dwProcessId,
                                                           const DWORD dwThreadId,
                                                           const ULONG ulProcessGroupId,
-                                                           _In_opt_ ConsoleProcessHandle* const pParentProcessData,


pParentProcessData wasn't used for anything except to change the return value.

lhecker · 2022-11-21T23:59:53Z

src/server/ProcessList.cpp

-            return E_FAIL;
-            // TODO: MSFT: 9574803 - This fires all the time. Did it always do that?
-            //RETURN_HR(E_FAIL);


There's only two callers. ApiDispatchers::ServerGenerateConsoleCtrlEvent always calls this with a valid pParentProcessData and won't ever run into this condition. IoDispatchers::ConsoleHandleConnectionRequest calls this method with a nullptr instead. This indicates that this error triggers, because we're getting redundant connection requests for the same process ID.

So because of that, this code block never gets called, right?

else { if (nullptr != ppProcessData) { *ppProcessData = pProcessData; } RETURN_HR(S_OK); }

Wouldn't we still want to keep it for correctness?

No it does get called by IoDispatchers::ConsoleHandleConnectionRequest. It sets pParentProcessData to nullptr which then invokes this branch. It does that so that AllocProcessData returns S_OK instead of E_FAIL if the given process ID already exists.

...but that practically makes the pParentProcessData parameter a bool allowDuplicateProcessId and I didn't particularly like that idea, when the caller might as well just filter the E_FAIL message themselves. This way the AllocProcessData is now way way simpler: It either sucessfully inserts a new process into the list or it fails. Simple as that! I felt like that would be a good idea.

lhecker · 2022-11-22T00:06:24Z

src/host/input.cpp

    {
        gci.UnlockConsole();
        return;
    }

    // Copy ctrl flags.
-    auto CtrlFlags = gci.CtrlFlags;
-    FAIL_FAST_IF(!(!((CtrlFlags & (CONSOLE_CTRL_CLOSE_FLAG | CONSOLE_CTRL_BREAK_FLAG | CONSOLE_CTRL_C_FLAG)) && (CtrlFlags & (CONSOLE_CTRL_LOGOFF_FLAG | CONSOLE_CTRL_SHUTDOWN_FLAG)))));


There's no good reason for this to fail-fast. We can just add a default: case to the switch case below, which I now did.

lhecker · 2022-11-22T00:08:16Z

src/server/ApiDispatchersInternal.cpp

+            if (FAILED(hr) && hr != E_FAIL)
+            {
+                RETURN_HR(hr);
+            }


Now that AllocProcessData consistently returns E_FAIL if the process ID already exists, we need this bit of extra code here. TBH I don't quite understand why this adds a "ProcessGroupId" as a process ID to the process list here...

carlos-zamora · 2022-11-28T18:41:53Z

src/server/ProcessList.cpp

-            return E_FAIL;
-            // TODO: MSFT: 9574803 - This fires all the time. Did it always do that?
-            //RETURN_HR(E_FAIL);


So because of that, this code block never gets called, right?

else { if (nullptr != ppProcessData) { *ppProcessData = pProcessData; } RETURN_HR(S_OK); }

Wouldn't we still want to keep it for correctness?

src/server/ProcessList.cpp

carlos-zamora · 2022-11-28T19:43:58Z

src/host/input.cpp

-        if (rgProcessHandleList[i].hProcess != nullptr)
-        {
-            CloseHandle(rgProcessHandleList[i].hProcess);
-        }


Don't we still need this in the event that if (NT_SUCCESS(Status)) fails?

I made it so that the hProcess is a wil::unique_handle so it'll get cleaned up automatically.

…rocess-list-refactor

DHowett · 2022-11-30T19:29:27Z

Can I add a validation step?

Launch cmd. Launch code inside it. exit in CMD once you get your prompt back. The window should stick around!

Who owns the window? My understanding of the code before your rewrite was that code.exe should. I'm not seeing that in practice however. Huh,

zadjii-msft · 2022-11-30T21:10:01Z

src/server/ProcessList.cpp

-        // Some applications, when reading the process list through the GetConsoleProcessList API, are expecting
-        // the returned list of attached process IDs to be from newest to oldest.
-        // As such, we have to put the newest process into the head of the list.


This was LOAD BEARING in the past. What happened to this?

answer: this moved to GetProcessList

DHowett

(my signoff is contingent on making sure the cmd => code => exit case works at least close to properly!)

…rocess-list-refactor

lhecker · 2022-12-01T23:51:27Z

@DHowett Did you have a specific invocation of VS Code in mind? The code executable only spawns the electron application and then exits, so we receive a disconnect event and ConsoleProcessList::FreeProcessData is called (I just confirmed it in a debugger).

DHowett

Minor correctness issues, but I love it nonetheless

src/interactivity/win32/windowio.cpp

src/server/ApiDispatchersInternal.cpp

src/server/ProcessList.h

src/server/ProcessList.cpp

DHowett · 2022-12-02T00:23:42Z

src/server/ProcessList.cpp

 // Arguments:
-// - dwProcessId - ID of the process to search for or ROOT_PROCESS_ID to find the root process.
+// - dwProcessId - ID of the process to search for.
 // Return Value:
 // - Pointer to the process handle information or nullptr if no match was found.
 ConsoleProcessHandle* ConsoleProcessList::FindProcessInList(const DWORD dwProcessId) const


you've validated that nobody calls Find(ROOT) even unintentionally now?

Yeah, there's only a single place calling this function with non-ROOT right now... It's ApiDispatchers::ServerGenerateConsoleCtrlEvent which calls it with valid process IDs.

DHowett · 2022-12-02T01:17:00Z

src/server/ProcessList.cpp

-    {
-        *ppProcessData = pProcessData;
-    }
+    wil::assign_to_opt_param(ppProcessData, pProcessData.release());


SCARY! This will delete the processdata if nobody asks for the optional output param

wait, no it won't. that's even scarier lol

ghost · 2022-12-02T23:15:25Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This commit is just a slight refactor of `ConsoleProcessList` which I've noticed was in a poor shape. It replaces iterators with for-range loops, etc. Additionally this fixes a bug in `SetConsoleWindowOwner`, where it used to default to the newest client process instead of the oldest. Finally, it changes the process container type from a doubly linked list over to a simple array/vector, because using linked lists for heap allocated elements felt quite a bit silly. To preserve the previous behavior of `GetProcessList`, it simply iterates through the vector backwards. * All unit/feature tests pass ✅ * Launching a TUI application inside pwsh inside cmd and exiting kills all 3 applications ✅ (cherry picked from commit 391abaf) Service-Card-Id: 87207823 Service-Version: 1.15

This commit is just a slight refactor of `ConsoleProcessList` which I've noticed was in a poor shape. It replaces iterators with for-range loops, etc. Additionally this fixes a bug in `SetConsoleWindowOwner`, where it used to default to the newest client process instead of the oldest. Finally, it changes the process container type from a doubly linked list over to a simple array/vector, because using linked lists for heap allocated elements felt quite a bit silly. To preserve the previous behavior of `GetProcessList`, it simply iterates through the vector backwards. ## Validation Steps Performed * All unit/feature tests pass ✅ * Launching a TUI application inside pwsh inside cmd and exiting kills all 3 applications ✅ (cherry picked from commit 391abaf) Service-Card-Id: 87207825 Service-Version: 1.16

ghost · 2022-12-14T20:43:13Z

🎉Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0 has been released which incorporates this pull request.:tada:

Handy links:

lhecker commented Nov 21, 2022

View reviewed changes

lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Nov 21, 2022

lhecker commented Nov 21, 2022

View reviewed changes

lhecker commented Nov 22, 2022

View reviewed changes

Refactor ConsoleProcessList

76a1367

lhecker force-pushed the dev/lhecker/console-process-list-refactor branch from 0b65033 to 76a1367 Compare November 22, 2022 14:42

carlos-zamora reviewed Nov 28, 2022

View reviewed changes

carlos-zamora requested review from zadjii-msft and DHowett November 28, 2022 19:44

lhecker added 2 commits November 30, 2022 17:51

Merge remote-tracking branch 'origin/main' into dev/lhecker/console-p…

e1bb5ef

…rocess-list-refactor

Address feedback

2aef629

lhecker force-pushed the dev/lhecker/console-process-list-refactor branch from b5aca7b to 2aef629 Compare November 30, 2022 16:52

This comment has been minimized.

Sign in to view

zadjii-msft approved these changes Nov 30, 2022

View reviewed changes

lhecker mentioned this pull request Dec 1, 2022

Give the root PID ownership of the pseudoconsole window #14196

Merged

DHowett requested changes Dec 1, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 1, 2022

lhecker added 2 commits December 2, 2022 00:48

Merge remote-tracking branch 'origin/main' into dev/lhecker/console-p…

6724ed1

…rocess-list-refactor

Fix spelling

1b967c3

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 1, 2022

DHowett requested changes Dec 2, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 2, 2022

Address feedback

c686b11

DHowett reviewed Dec 2, 2022

View reviewed changes

DHowett approved these changes Dec 2, 2022

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 2, 2022

ghost merged commit 391abaf into main Dec 2, 2022

ghost deleted the dev/lhecker/console-process-list-refactor branch December 2, 2022 23:16

DHowett added this to To Cherry Pick in 1.16 Servicing Pipeline via automation Dec 12, 2022

DHowett added this to To Cherry Pick in 1.15 Servicing Pipeline via automation Dec 12, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.15 Servicing Pipeline Dec 12, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.16 Servicing Pipeline Dec 12, 2022

DHowett moved this from Cherry Picked to Shipped in 1.16 Servicing Pipeline Jan 13, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ConsoleProcessList #14421

Refactor ConsoleProcessList #14421

lhecker commented Nov 21, 2022 •

edited

lhecker Nov 21, 2022 •

edited

KalleOlaviNiemitalo Nov 22, 2022

KalleOlaviNiemitalo Nov 22, 2022

lhecker Nov 22, 2022

lhecker Nov 21, 2022

lhecker Nov 21, 2022 •

edited

carlos-zamora Nov 28, 2022

lhecker Nov 30, 2022

lhecker Nov 22, 2022 •

edited

lhecker Nov 22, 2022

carlos-zamora Nov 28, 2022

carlos-zamora Nov 28, 2022

lhecker Nov 30, 2022

This comment has been minimized.

DHowett commented Nov 30, 2022 •

edited

zadjii-msft Nov 30, 2022

zadjii-msft Nov 30, 2022

DHowett left a comment

lhecker commented Dec 1, 2022

DHowett left a comment

DHowett Dec 2, 2022

lhecker Dec 2, 2022

DHowett Dec 2, 2022

DHowett Dec 2, 2022

ghost commented Dec 2, 2022

ghost commented Dec 14, 2022

Refactor ConsoleProcessList #14421

Refactor ConsoleProcessList #14421

Conversation

lhecker commented Nov 21, 2022 • edited

Validation Steps Performed

lhecker Nov 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

DHowett commented Nov 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented Dec 1, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 2, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Dec 14, 2022

lhecker commented Nov 21, 2022 •

edited

lhecker Nov 21, 2022 •

edited

lhecker Nov 21, 2022 •

edited

lhecker Nov 22, 2022 •

edited

DHowett commented Nov 30, 2022 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.