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

containers/attach: Note bug around goroutine leak #23233

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

cgwalters
Copy link
Contributor

See:

Sorry for not trying to fix this, but I am not aware of a remotely straightforward way to do so.

None

@cgwalters
Copy link
Contributor Author

I am not aware of a remotely straightforward way to do so.

Searching around a bit https://groups.google.com/g/golang-nuts/c/4ZrDw__VkyI looks like the most related thread. I can imagine doing that, basically drop down to a select/poll syscall on the fd, and have that send a message on a channel when ready; the other goroutine selects on that and the cancellation context. When the select chan is ready we do a single read+write cycle.

In the end, it is just simpler and clearer for us to just fork off podman as a binary which fully papers over this bug, but maybe someone else who wants to fix this can find this PR discussion later and do it.

@cgwalters cgwalters added the No New Tests Allow PR to proceed without adding regression tests label Jul 9, 2024
@Luap99
Copy link
Member

Luap99 commented Jul 9, 2024

Yeah this is really the ugly parts about go that you cannot cancel a read without closing the stream.

Also your tree is outdated please rebase on the latest main

@Luap99
Copy link
Member

Luap99 commented Jul 9, 2024

select/poll syscall on the fd

Yeah that would make it ugly as we have to support the bindings on windows as well. So then we woul dend up with OS specific code...

See:

- containers/podman-bootc#59 (comment)
- containers/podman-bootc#61

Sorry for not trying to fix this, but I am not aware of
a remotely straightforward way to do so.

Signed-off-by: Colin Walters <[email protected]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented Jul 9, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 345bac6 into containers:main Jul 9, 2024
83 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 8, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants