Skip to content
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

Make sure we fully clean up state when closing a peasant #11217

Merged
2 commits merged into from
Sep 14, 2021

Conversation

Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Sep 13, 2021

Fix infinite loop when trying to summon a window after close.

In #10972 code was added to try to clean up state manually when a window
was closed instead of waiting for it to be detected as a dead peasant.
Unfortunately I didn't know any better and missed cleaning up
_mruPeasants as well. The result is there would be an infinite loop
in _getMostRecentPeasant since _getPeasant will only clean up ids if
it finds a peasant, not if it doesn't find anything. This is the minimal
change to get this working, but it might be a good idea to make
_getPeasant be more thorough about cleanup.

Validation Steps Performed

Tested that before the change we infinitely loop, and after the change
we summon correctly.

Closes #11215

@ghost ghost added Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Sep 13, 2021
@Rosefield
Copy link
Contributor Author

Rosefield commented Sep 13, 2021

@zadjii-msft I added a test for this particular case. I confirmed that the test runs forever (or at least until I killed it) without the code change, and with the change it passes. It is identical to TestSummonOnCurrentDeadWindow except it calls _closePeasant instead of _killPeasant.

In the future it might be worth evaluating in the tests if they should use _closePeasant instead of _killPeasant but that seems out of scope for this bug fix.

@zadjii-msft zadjii-msft self-assigned this Sep 13, 2021
@Rosefield
Copy link
Contributor Author

Being top contributor to this project is so easy

  1. Make large sweeping changes
  2. Find a bunch of bugs that are a result of (1.)
  3. Fix them in individual PRs
  4. ???
  5. Profit.

@zadjii-msft
Copy link
Member

  • Make large sweeping changes
  • Find a bunch of bugs that are a result of (1.)

you've basically described my entire time on the Console team

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks!

@DHowett
Copy link
Member

DHowett commented Sep 13, 2021

@msftbot merge this in 4 minutes

@ghost
Copy link

ghost commented Sep 13, 2021

Hello @DHowett!

I think you told me that you want to delay the approval for a certain amount of time, but I am not confident that I have understood you correctly.

Please try rephrasing your instruction to me.

@DHowett
Copy link
Member

DHowett commented Sep 13, 2021

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 13, 2021
@ghost
Copy link

ghost commented Sep 13, 2021

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Mon, 13 Sep 2021 19:50:24 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests here. We may (in general) just need more tests now that a peasant can close gracefully rather than just "no longer exist", but this will probably cover the biggest case. Thanks!

@ghost ghost merged commit b4a40ff into microsoft:main Sep 14, 2021
@Rosefield Rosefield deleted the bug/gh11215-mru-peasant-hang branch September 14, 2021 15:38
@ghost
Copy link

ghost commented Oct 20, 2021

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

Handy links:

This pull request was closed.
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. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mysterious hang in Monarch::_getMostRecentPeasantID
3 participants