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

Fix cleanup code #626

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Fix cleanup code #626

merged 2 commits into from
Oct 4, 2023

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Aug 25, 2023

Fixes #512

@rzhao271 rzhao271 self-assigned this Aug 25, 2023
@rzhao271 rzhao271 requested a review from Tyriar August 25, 2023 19:27
@rzhao271 rzhao271 added this to the September 2023 milestone Aug 25, 2023
@rzhao271
Copy link
Contributor Author

I have confirmed that I have moved the code to a part of the file that is called later:

  • WindowsPtyAgent#ctor calls into PtyStartProcess and PtyConnect
  • PtyConnect sets up a process exit listener that calls OnProcessExit
  • PtyKill closes the hShell handle, but OnProcessExit is the part of the code that actually reads hShell's exit code, so I'm thinking that OnProcessExit is always run later than PtyKill

I still have to test this PR, though.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Aug 25, 2023

I have tested the PR
DisconnectNamedPipe causes the crash. I have removed those calls for now and added a comment referencing the issue.

Edit: DisconnectNamedPipe fails with error ERROR_INVALID_HANDLE. I wonder whether that means that after pfnClosePseudoConsole is called, the hIn and hOut handles are somehow invalidated?

Edit: I ran another experiment where I moved DisconnectNamedPipe back to the PtyKill implementation, and I got the error ERROR_ACCESS_DENIED.

@rzhao271 rzhao271 marked this pull request as ready for review August 25, 2023 20:47
@rzhao271 rzhao271 modified the milestones: September 2023, August 2023 Aug 25, 2023
@rzhao271 rzhao271 changed the title Move disconnect code to OnProcessExit Fix cleanup code Aug 25, 2023
@rzhao271 rzhao271 modified the milestones: August 2023, September 2023 Aug 28, 2023
@rzhao271 rzhao271 modified the milestones: September 2023, October 2023 Oct 3, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on latest beta
2 participants