fix #6624 can't start new thread #6653

kalefranz · 2018-01-04T21:02:35Z

This PR removes the backdown_thread_pool context manager, and replaces it with ThreadLimitedThreadPoolExecutor. The original purpose of backdown_thread_pool was to prevent RuntimeErrors as shown in #6624, but the strategy failed to do so. After looking more at concurrent.futures.ThreadPoolExecutor, it seems the exception will only be thrown when a new thread is created, and that only happens after submitting the first task. So backdown_thread_pool was wholly ineffective at its intended purpose.

This PR has some ugly parts, things I complain about in comments. Specifically import and usage of "protected" attributes (names starting with _) from concurrent.futures. That seems unavoidable though. The only solution there would be to vendor our own concurrent.futures. I'm indifferent on whether we want to do that or not.

I'd like to have two reviewers on this PR. Probably @mingwandroid since he's already taken a first pass at it, and also @goanpeca.

kalefranz · 2018-01-05T17:43:42Z

conda/common/io.py

@@ -395,16 +395,43 @@ def backdown_thread_pool(max_workers=10):
    """Tries to create an executor with max_workers, but will back down ultimately to a single
    thread of the OS decides you can't have more than one.
    """
+    from concurrent.futures import _base  # These "_" imports are gross, but I don't think there's an alternative  # NOQA
+    from concurrent.futures.thread import _WorkItem


Need to make sure these imports are ok for python 3.4 and python 3.5. They obviously are for python 2.7 and 3.6.

Confirmed ok for both py34 and py35.

kalefranz · 2018-01-05T17:53:54Z

conda/common/io.py

+
+    class CondaThreadPoolExecutor(ThreadPoolExecutor):
+
+        def submit(self, fn, *args, **kwargs):


This is an exact reimplementation of the submit() method on the parent class, except I've added try/except around self._adjust_thread_count(). Seems to me like this is the technically sound and correct solution. I don't have any independent references though, and I'm frustrated it was so dirty to implement.

Maybe if you included a link to the exact lines you copied on github (as a comment in the source) it'd make it easy to see if it's changed indicating you need to update your modified version?

kalefranz · 2018-01-10T14:43:34Z

BTW, at the moment tests are running, but they've all passed previously. I force-pushed only to correct some cosmetic things in docstrings.

goanpeca · 2018-01-10T15:14:35Z

conda/common/io.py

+            return f
+
+
+as_completed = as_completed  # anchored here for import by other modules


Why do we need this here, if it is imported anyway on the top?

It's for flake8 so that we don't have an unused import.

ahhh right... you can just do

as_completed

for that, no ?

No, because then PyCharm complains.

goanpeca · 2018-01-10T15:17:37Z

conda/common/io.py

+                # RuntimeError: can't start new thread
+                # See https://github.com/conda/conda/issues/6624
+                if len(self._threads) > 0:
+                    # It's ok to not be able to start new threads if we already have at least


Ok so this looks like it will prevent a lot of issues, but maybe we can be honest that it will still occur (less likely though).

Maybe we need to add some python minor/patch version check and add some warning if it is not using the version for which this is tested. Or we could vendor (as@kalefranz suggest) it if it is small enough. I am just worried we inadvertedly fail due to api changes?

Maybe we should vendor this as part of this PR already and avoid future issues?

Maybe we should vendor this as part of this PR already and avoid future issues?

Much prefer that to some type of version checking. I'll vendor it in this PR if @mingwandroid agrees.

Ok so this looks like it will prevent a lot of issues, but maybe we can be honest that it will still occur (less likely though).

Yeah, if no threads can be created at all, then we have further work to do. I don't want to handle that case though until we actually see evidence that it exists.

I've looked at code for concurrent.futures in all the various locations. I'd rather not vendor right now. We'll of course test conda with python 3.7 before it's released, and other future versions. We're running all the unit tests now for every built conda package as part of the meta.yaml recipe, and given that we build python-specific versions of conda (not noarch), I think that mitigates the concerns for why we'd vendor.

Ok, makes sense

goanpeca

LGTM, left some minor comments

kalefranz · 2018-01-10T16:24:31Z

After this PR is approved and merged, I think there's some refactoring in order for the conda/common/io.py module. It's starting to be a collection of a lot of unrelated stuff.

github-actions · 2021-09-04T06:02:07Z

Hi there, thank you for your contribution to Conda!

This pull request has been automatically locked since it has not had recent activity after it was closed.

Please open a new issue or pull request if needed.

kalefranz added the Ready for Review label Jan 5, 2018

kalefranz added this to the 4.4 milestone Jan 5, 2018

kalefranz commented Jan 5, 2018

View reviewed changes

fix conda#6624 can't start new thread

ec2907a

kalefranz force-pushed the fix-6624-cannot-start-thread branch from 5b2f144 to 95f66bc Compare January 10, 2018 02:59

kalefranz added 2 commits January 9, 2018 21:02

ThreadLimitedThreadPoolExecutor

f73822d

fix typo

159645e

kalefranz force-pushed the fix-6624-cannot-start-thread branch from 95f66bc to 159645e Compare January 10, 2018 14:32

kalefranz requested a review from goanpeca January 10, 2018 14:40

goanpeca reviewed Jan 10, 2018

View reviewed changes

goanpeca approved these changes Jan 10, 2018

View reviewed changes

kalefranz modified the milestones: 4.4, 4.4.8 Jan 13, 2018

kalefranz mentioned this pull request Jan 22, 2018

fix a failure with multi-threaded repodata downloads #3078

Merged

goanpeca merged commit c1af02b into conda:4.4.x Jan 23, 2018

kalefranz mentioned this pull request Jan 25, 2018

conda update breaks conda #6787

Closed

1 task

kalefranz mentioned this pull request Feb 9, 2018

RuntimeError: cannot join thread before it is started #6623

Closed

wilsonmr mentioned this pull request Mar 15, 2018

conda install RuntimeError: can't start new thread #7040

Closed

2 tasks

kalefranz deleted the fix-6624-cannot-start-thread branch March 16, 2018 15:08

kalefranz mentioned this pull request Mar 16, 2018

RuntimeError: can't start new thread tqdm/tqdm#522

Closed

github-actions bot added the locked [bot] locked due to inactivity label Sep 4, 2021

github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2021


		class CondaThreadPoolExecutor(ThreadPoolExecutor):

		def submit(self, fn, args, *kwargs):

		return f


		as_completed = as_completed # anchored here for import by other modules

fix #6624 can't start new thread #6653

fix #6624 can't start new thread #6653

Uh oh!

Conversation

kalefranz commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingwandroid Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalefranz commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalefranz Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goanpeca left a comment

Choose a reason for hiding this comment

Uh oh!

kalefranz commented Jan 10, 2018

Uh oh!

github-actions bot commented Sep 4, 2021

Uh oh!

Uh oh!

kalefranz commented Jan 4, 2018 •

edited

Loading

mingwandroid Jan 5, 2018 •

edited

Loading

kalefranz Jan 10, 2018 •

edited

Loading