Skip to content

fix: descriptor leaking in salt.utils.http.query#68456

Open
vitalvas wants to merge 3 commits into
saltstack:3007.xfrom
vitalvas:master
Open

fix: descriptor leaking in salt.utils.http.query#68456
vitalvas wants to merge 3 commits into
saltstack:3007.xfrom
vitalvas:master

Conversation

@vitalvas

@vitalvas vitalvas commented Nov 12, 2025

Copy link
Copy Markdown

fix: descriptor leaking in salt.utils.http.query

Fix file descriptor leak in salt.utils.http.query by wrapping SyncWrapper(AsyncHTTPClient, ...) in a context manager so the temporary IOLoop + socketpair are reliably torn down on both success and HTTP error paths.

Adopted from upstream onto current 3007.x by @dwoz; original author @vitalvas preserved on the 3 cherry-picked commits (fe44e3fc5c9, fe3d82bc252, 147d20f645b).

Fixes #68111

@vitalvas vitalvas requested a review from a team as a code owner November 12, 2025 17:24
@welcome

welcome Bot commented Nov 12, 2025

Copy link
Copy Markdown

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject.pdl@broadcom.com. We're glad you've joined our community and look forward to doing awesome things with you!

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great. Could you create a changelog?

@dwoz

dwoz commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Is this issue on 3006.x or 3007x too?

@vitalvas

vitalvas commented Nov 14, 2025

Copy link
Copy Markdown
Author

Is this issue on 3006.x or 3007x too?

I only caught this bug in 3007.x.

@twangboy Added to changelog.

Can we add this patch to release in 3007.x?

@vitalvas vitalvas requested a review from twangboy November 14, 2025 16:16
@twangboy

Copy link
Copy Markdown
Contributor

We usually patch bugs on the earliest supported branch where the bug exists. If this bug exists on 3006.x, please patch it there, then the changes will be merged forward into 3007.x and master branches.

@vitalvas

Copy link
Copy Markdown
Author

This bug was introduced in #66330.

I don't see the same issue in 3006.x releases.

@dwoz

dwoz commented Nov 18, 2025

Copy link
Copy Markdown
Contributor

This bug was introduced in #66330.

I don't see the same issue in 3006.x releases.

Ideally the fix would go into the 3007.x branch then.

@vitalvas vitalvas changed the base branch from master to 3007.x November 18, 2025 18:56
@twangboy

Copy link
Copy Markdown
Contributor

Please address the merge conflicts

@twangboy

Copy link
Copy Markdown
Contributor

Please rebase and address the merge conflicts

@bdrx312

bdrx312 commented May 1, 2026

Copy link
Copy Markdown
Contributor

@vitalvas Can you rebase this?

@dwoz

dwoz commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Please add Fixes #68111 to the PR body so this auto-closes the issue on merge. The leak path you describe is exactly what #68111 reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants