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

internal/exec: Fix stdio pipe problems #2021

Merged
merged 1 commit into from Feb 12, 2024
Merged

Conversation

kevpar
Copy link
Member

@kevpar kevpar commented Feb 8, 2024

exec today has two problems with how it handles stdio pipes:

  • When Wait completes, closeStdio() is called. This closes the parent-side stdio pipes for receiving IO from the process. This is a problem because once the process has completed, we still need to be able to receive any final output. Today data from the process could be lost because of this.
  • The parent's handles to the child-side stdio pipes are not closed after starting the process. Leaving duplicates of these handles in the parent process means that the other ends of the pipes are never closed when the process exits.

This commit makes the following changes:

  • The parent's handles to the child-side stdio pipes are now closed after the child is started. This is necessary so that once the child exits, the parent-side pipes will return EOF once the remaining output drains.
  • When Wait completes, the parent-side stdio pipes are not closed. The responsibility for this is now left to the client of the exec package. Currently the only user of exec is jobcontainers.JobProcess, which closes handles these when Close is called.

Additionally, the ProcThreadAttributeList is now allocated and used only in Start. Previously it was saved on the Exec object, even though it was not needed elsewhere. This makes the code cleaner, simplifies the Wait logic, and eliminates the chance of leaking memory if an Exec object is GC'd without being Wait'd.

exec today has two problems with how it handles stdio pipes:

- When Wait completes, closeStdio() is called.
  This closes the parent-side stdio pipes for receiving IO from the
  process. This is a problem because once the process has completed, we
  still need to be able to receive any final output. Today data from the
  process could be lost because of this.
- The parent's handles to the child-side stdio pipes are not closed
  after starting the process. Leaving duplicates of these handles in the
  parent process means that the other ends of the pipes are never closed
  when the process exits.

This commit makes the following changes:

- The parent's handles to the child-side stdio pipes are now closed
  after the child is started. This is necessary so that once the child
  exits, the parent-side pipes will return EOF once the remaining output
  drains.
- When Wait completes, the parent-side stdio pipes are not closed. The
  responsibility for this is now left to the client of the exec package.
  Currently the only user of exec is jobcontainers.JobProcess, which
  closes handles these when Close is called.

Additionally, the ProcThreadAttributeList is now allocated and used only
in Start. Previously it was saved on the Exec object, even though it was
not needed elsewhere. This makes the code cleaner, simplifies the Wait
logic, and eliminates the chance of leaking memory if an Exec object
is GC'd without being Wait'd.

Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
@msscotb msscotb self-requested a review February 9, 2024 22:14
@kevpar kevpar merged commit ab6e48b into microsoft:main Feb 12, 2024
18 of 19 checks passed
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.

None yet

3 participants