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 dangling fd with stdio #287

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

jprendes
Copy link
Collaborator

@jprendes jprendes commented Aug 29, 2023

With #260 the stdio fd are dangling in the parent shim process.
Normally redirecting the stdio would clear the fds.
However, the redirection happens in the clone3d child process.
This means that the parent process maintains a dangling cope of the ´fd´s, keps alive by the shim Instances.

The key changes in this PR are:

  • Stdio now offers a take method.
  • The Instance impls now use stdio.take() for the last executor, giving up ownership of the fds.
  • Adds a smoke test that used to hang without this change.

The rest of the changes are general fixes:

  • Change the Muxtex in Stdio to an AtomicCell (we need two nested ones to be able to implement ´take´)
  • Add timeouts to the tests in CI
  • Update the release workflow, which was using the old path of a moved script
  • Replace the macro in Stdio with a generic const.
  • implement TryFrom<&InstanceConfig<Engine>> for Stdio

@jprendes jprendes force-pushed the fix-fd-issue branch 4 times, most recently from 1f1c9e2 to 70231f3 Compare August 29, 2023 13:49
@jprendes jprendes closed this Aug 29, 2023
@jprendes jprendes reopened this Aug 29, 2023
@jprendes jprendes requested a review from Mossaka August 29, 2023 14:21
@jprendes jprendes marked this pull request as ready for review August 29, 2023 14:24
@jprendes
Copy link
Collaborator Author

@CaptainVincent , this is an alternative fix to #280.

Mossaka
Mossaka previously approved these changes Aug 29, 2023
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm!

@jprendes
Copy link
Collaborator Author

Rebased to account for #290.
The fact that we now have only one executor simplifies things and we don't need the nested Arc<AtomicCell<...>>.

@jprendes
Copy link
Collaborator Author

@Mossaka @jsturtevant PTAL.
If possible I would like to have this merged before the end of the week.

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

LGTM, few comments to help my understanding

Mossaka
Mossaka previously approved these changes Aug 31, 2023
Signed-off-by: Jorge Prendes <[email protected]>
@Mossaka Mossaka merged commit 7e978ed into containerd:main Aug 31, 2023
@jprendes jprendes deleted the fix-fd-issue branch September 6, 2023 11:59
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.

3 participants