Skip to content

Ensure Process.MainWindowTitle and Process.Responding is refreshed - #38385

Merged
eiriktsarpalis merged 7 commits into
dotnet:masterfrom
JeroenOortwijn:Fix-issue-36768-Process.MainWindowTitle-and-Process.Responding-are-not-correctly-refreshed
Aug 12, 2020
Merged

Ensure Process.MainWindowTitle and Process.Responding is refreshed#38385
eiriktsarpalis merged 7 commits into
dotnet:masterfrom
JeroenOortwijn:Fix-issue-36768-Process.MainWindowTitle-and-Process.Responding-are-not-correctly-refreshed

Conversation

@JeroenOortwijn

Copy link
Copy Markdown
Contributor

PR for issue #36768.

It should fix the issue, but I'm unable to run the tests because opening the solution leads to the following NuGet error:

Error occurred while restoring NuGet packages: Invalid restore input. Duplicate frameworks found: 'net5.0, net5.0, net5.0, net5.0, net5.0, net5.0'. Input files: E:\Repos\JeroenOortwijn\dotnet\runtime\src\libraries\System.Diagnostics.Process\src\System.Diagnostics.Process.csproj.

(Which, I think, is tracked in #34173.)

@ghost

ghost commented Jun 25, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

@dnfadmin

dnfadmin commented Jun 25, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question; why do we expect Responding to be false after a Refresh?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this behaviour with the test program that I've written:
https://github.com/JeroenOortwijn/Responding-test

With .Net Framework 4.8, the first Process.Responding call (immediately after Wordpad is started) returns true.
After a couple of seconds, Wordpad is so busy loading the document that the title bar shows "(Not Responding)". At that point, Process.Responding returns false.

Due to issue #36768, Process.Responding keeps returning true on .Net Core 3.1 and .Net 5.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woh, this is creating a 2GB temporary file? Even in outerloop that's probably something to be avoided.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the file is quite big.
But I want to make sure that even on the fastest machine, Wordpad goes into a "not responding" state.
If the file is too small, Wordpad will have loaded the file without getting into that state.

So, some kind of optimum file size has to be found.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really need to test that, I suggest the test itself needs to be the thing that causes the non-responsiveness, e.g. the test would RemoteExecutor.Invoke code that would create a window that's not responsive. We shouldn't depend on creating a big enough file that the particular implementation in wordpad causes the UI to hang.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub you mean with something like windows forms? I tried the following

static void RunNotRespondingApp()
{
    var form = new System.Windows.Forms.Form();
    form.Show();
    Thread.Sleep(60_000);
}

RemoteExecutor.Invoke(RunNotRespondingApp);

but for whatever reason it wasn't enough to simulate not responding state using RemoteExecutor. It does work without issue when I run as a standalone app.

@adamsitnik adamsitnik added this to the 5.0.0 milestone Jun 25, 2020
@adamsitnik

Copy link
Copy Markdown
Member

@stephentoub could you please respond to @JeroenOortwijn answers to your review comments?

@eiriktsarpalis
eiriktsarpalis force-pushed the Fix-issue-36768-Process.MainWindowTitle-and-Process.Responding-are-not-correctly-refreshed branch from 5eef234 to 18f3c31 Compare August 10, 2020 13:11
@eiriktsarpalis

eiriktsarpalis commented Aug 11, 2020

Copy link
Copy Markdown
Member

I rebased and ran the tests locally. Can confirm that the changes fix the behaviour (matching it with .NET Framework behaviour). I would recommend merging this PR ahead of the 5.0 deadline, but we could perhaps disable the problematic test (it's not run in CI anyway, and we generally have no test coverage for Process.Responding). cc @adamsitnik @danmosemsft

@danmoseley

Copy link
Copy Markdown
Contributor

I would recommend merging this PR ahead of the 5.0 deadline, but we could perhaps disable the problematic test

Seems reasonable to me.

@eiriktsarpalis

Copy link
Copy Markdown
Member

Thanks @JeroenOortwijn for your contribution!

Fixes #36768.

@danmoseley

Copy link
Copy Markdown
Contributor

@dotnet/dnceng @jaredpar timeouts on “ Build OSX x64 release Runtime_Debug” and similar on a lot of.PRs right now having a significant impact. Can you help us understand why?

@jaredpar

Copy link
Copy Markdown
Member

@danmosemsft

Looking through the logs it seems like the problem is the C++ build is just taking about 70 minutes to complete on OSX. That is eating up most of the available time for the job.

@jakubstilec

Copy link
Copy Markdown

@danmosemsft checking now

@jakubstilec

Copy link
Copy Markdown

Job timed out but it wasn't helix machine, but azdo macos machine.

@jakubstilec

Copy link
Copy Markdown

I don't see any active issue in azdo. Retried the job and checking.

@eiriktsarpalis
eiriktsarpalis merged commit 7d25902 into dotnet:master Aug 12, 2020
@jakubstilec

Copy link
Copy Markdown

@danmosemsft now macox build job "Build product" has finished in ~56 minutes, previously it timed out in 2 hours. It seems that it was a temporary issue on AzDO side.

@jaredpar

Copy link
Copy Markdown
Member

@eiriktsarpalis why are we merging changes when the build legs are timing out?

@eiriktsarpalis

eiriktsarpalis commented Aug 12, 2020

Copy link
Copy Markdown
Member

@jaredpar Apologies, I assumed by the conversation that you were discussing infra issues. Do you need me to revert?

FWIW all tests were green on the initial run. The last commit was me deleting a test.

@jaredpar

Copy link
Copy Markdown
Member

@jakubstilec latest seems to indicate it's working again so lets keep for now and willmonitor

@MattGal

MattGal commented Aug 12, 2020

Copy link
Copy Markdown
Member

@danmosemsft

Looking through the logs it seems like the problem is the C++ build is just taking about 70 minutes to complete on OSX. That is eating up most of the available time for the job.

We've moving from MacStadium to GitHub-hosted machines; The new machines have fewer cores. I believe the only option currently is to increase your timeouts but, as usual, I know who you can engage in conversation if you think you can talk them into making beefier mac vms. edit: this is theoretically only true for internal builds; that said they know the new machines are slower in general.

@JeroenOortwijn
JeroenOortwijn deleted the Fix-issue-36768-Process.MainWindowTitle-and-Process.Responding-are-not-correctly-refreshed branch August 12, 2020 18:14
@danmoseley

Copy link
Copy Markdown
Contributor

@MattGal thanks. I think that's for @jaredpar to decide on based on monitoring he mentions..

@MattGal

MattGal commented Aug 12, 2020

Copy link
Copy Markdown
Member

@MattGal thanks. I think that's for @jaredpar to decide on based on monitoring he mentions..

Yes I did quote reply of him but it looked like I was talking at you, sorry. The key takeaway is they know the new hardware is slower and they're not going back to the old hardware.

@danmoseley

Copy link
Copy Markdown
Contributor

@jaredpar is it simple to increase our timeouts? If so perhaps we should do that pre-emptively, given we expect CI to stay busy for the near future?

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants