Switch resolver to threaded variant #23164
Conversation
… uses blocking implementation
|
I created additional pull request to jenkins-ci-states vmware-archive/jenkins-ci-states#2 It is required to test this pull request since new dependency (futures) is added. Also should I update doc/ files? |
|
@vladimir-didenko, documentation and test updates are very welcome, but not required. It would be great if you will also update the failing tests according to your code changes, thanks. |
|
Thanks for the fix! As mentioned in my previous PR (I think?) this actually shouldn't be an issue for us because salt-minion does the DNS resolution before getting to the channels (https://github.com/saltstack/salt/blob/develop/salt/minion.py#L114), but this will be useful once we finally move that logic into the channels (where it belongs). |
|
@jfindlay, as @vladimir-didenko mentioned before it's necessary to merge jenkins-ci-states#2 first to make jenkins to handle dependencies correctly. |
|
I could be confused, but our production jenkins currently runs states from https://github.com/saltstack/salt-jenkins.git, not https://github.com/saltstack/jenkins-ci-states.git, so a pull request against salt needing updates to the testing environment states will have to make changes to salt-jenkins so that the tests run by github will pass. |
|
Once we figure out where this dep needs to go this can be merged! |
|
@jfindlay @thatch45 I've added futures dependecy to salt-jenkins project for production builds. |
|
Jenkins-ci-states is the next, not yet ready, Jenkins state tree. Sorry for the confusion. |
|
Anyway, PRs for both are ready. Just reject those that aren't needed. |
|
No, no need to reject 😄, both will be needed... |
|
Glad to hear it! =) Thank you! |
|
saltstack/salt-ci-images#96 has been merged. |
|
All that remains here is some unrelated lint. |
Switch resolver to threaded variant
Hi, guys.
We noticed that your developers tried to fix blocking DNS resolving in Tornado TCP client
9c68051
But actually this commit doesn't fix issue. Fix configures tornado.netutil.ExecutorResolver for using as default DNS Resolver. But it is useful only when you provide own 'executor=' param to constructor. TCP Client creates resolver without additional params so ExecutorResolver uses dummy_executor (DummyExecutor) (see http://tornado.readthedocs.org/en/latest/_modules/tornado/concurrent.html) that simply calls function in synchronous blocking mode. It means that your Salt application still hangs in DNS resolving call and will be unresponsive to all events until resolving is finished.
Also there is a strange comment inside commit
ThreadedResolver works fine with python2. You only need to add dependency on 'futures' package https://pypi.python.org/pypi/futures - it is backport of concurrent.futures package from python3.