Fix 58141 - ThreadPool leak in fqdn lookup #61777
Conversation
I wasn't positive what other errors could be raised here, but we did have a user reporting thread leaks, and the existing code *could* have thread leaks if unhandled exceptions are raised on the ThreadPool. This test *should* work - there might be some weird circumstances where other threads are magically closed between here? But that seems less likely.
While working on making the ThreadPool fixes I discovered a slight lack of tests and opportunities for refactoring. These tests pass with the existing setup, and will help verify that my refactoring changes do not harm anything.
Before if a unaccounted for error happened during fqdn lookup then the ThreadPool would exit without cleaning up. If, for instance you have a malformed PTR record then something bad would happen with the lookup and the fqdn function would exit without cleaning up the ThreadPool. This fixed saltstack#59705 which had this exact problem. I refactored this to use ThreadPoolExecutor which will actually cleanup the ThreadPool on with-block exit. I also put a broad exception handler in the fqdn internal function. This will prevent all but the most egregious errors (i.e. ones that don't inherit from Exception - e.g. SystemExit) from crashing the fqdn lookup process. Also took a couple of opportunities for refactor, such as the fact that `sorted` takes any iterable and returns a list, so I could remove that double-call.
|
this is blocked by #61919 |
|
Other failures appear to just be the test machines failing 🤷 Will update when blocker is merged |
|
Just merged in the test failures, hopefully that sorts all the test failures |
Hi! I'm your friendly PR bot!You might be wondering what I'm doing commenting here on your PR. Yes, as a matter of fact, I am... I'm just here to help us improve the documentation. I can't respond to Okay... so what do you do? I detect modules that are missing docstrings or "CLI Example" on existing docstrings! So what does that have to do with my PR? I noticed that in this PR there are some files changed that have some of these Okay, what are they? Well, my favorite, is that since you were making changes here I'm hoping that If I can, then what? Well, you can either add them to this PR or add them to another PR. Either way is fine! Well... what if I can't, or don't want to? That's also fine! We appreciate all contributions to the Salt Project. If you Whatever approach you decide to take, just drop a comment here letting us know! Detected Issues (click me)Check Known Missing Docstrings...........................................Failed - hook id: invoke - duration: 1.7s - exit code: 1 Thanks again! |
What does this PR do?
What it says on the tin.
What issues does this PR fix or reference?
Fixes #58141
Previous Behavior
If an error occurred within the inner fqdn lookup function then the ThreadPool
would not cleanup, causing a thread leak every time the fqdn lookup happened.
Which could have been a lot, since grains uses this lookup. Especially since
the grain lookup may have crashed, so every call would require a re-compute of
the fqdn.
New Behavior
Changed to ThreadPoolExecutor which has the same API except that it can be used
in a
withblock to automagically ensure tha the ThreadPool is cleaned up whenit's done.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
- [ ] DocsCommits signed with GPG?
Yes