Repository: conan-io/conan
Pull request: [bugfix] multiple remotes with single conan upload
Changelog: Bugfix: Fixed bug where uploading to multiple remotes in a single conan upload command would fail.
Docs: omit

Fixes #7780 

This bug was due to closing and reusing a thread pool in a loop. Instead, a separate thread pool will be created at each loop iteration.

- [x] Refer to the issue that supports this Pull Request.
- [x] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
- [x] I've read the [Contributing guide](https://github.com/conan-io/conan/blob/develop/.github/CONTRIBUTING.md).
- [x] I've followed the PEP8 style guides for Python code.
- [ ] I've opened another PR in the Conan docs repo to the ``develop`` branch, documenting this one. 

<sup>**Note:** By default this PR will skip the slower tests and will use a limited set of python versions. Check [here](https://github.com/conan-io/conan/blob/develop/.github/PR_INCREASE_TESTING.md) how to increase the testing level by writing some tags in the current PR body text.</sup>


Problem statement:
[bug] Upload fails with multiple remotes in the same command
When uploading several packages at once (without specifying a remote) and some packages are uploaded to different remotes, the command will fail.

The bug appears to be due to reusing a closed thread pool in the loop in CmdUploader.upload() in uploader.py. Once thread pools are closed they can't be reused, so any iterations after the first will cause an error.

### Environment Details (include every applicable attribute)
  * Operating System+version: RHEL 7.6
  * Conan version: Tested with 1.29.0 and 1.30.dev0 (develop)
  * Python version: 3.8.5

### Steps to reproduce (Include if Applicable)
1. Set up two remotes
2. Create 2 recipes and add_ref one recipe to each remote
4. Upload using `conan upload "*" --all`

### Logs (Executed commands with output) (Include/Attach if Applicable)
I can't get the logs off my machine, but the stack trace looks like this:
command.py:2103
command.py:1477
conan_api.py:94
conan_api.py:965
uploader.py:109
pool.py:364
pool.py:473
pool.py:350
ValueError: Pool not running

Hints from issue discussion:

Commit message: Fixed bug where uploading to multiple remotes in a single conan upload command would fail

Patch to review:
diff --git a/conans/client/cmd/uploader.py b/conans/client/cmd/uploader.py
index f47631f397b..2e7e877f47f 100644
--- a/conans/client/cmd/uploader.py
+++ b/conans/client/cmd/uploader.py
@@ -201,12 +201,11 @@ def upload(self, reference_or_pattern, remotes, upload_recorder, package_id=None
                                            all_packages, query)
 
         if parallel_upload:
-            self._upload_thread_pool = ThreadPool(8)
             self._user_io.disable_input()
-        else:
-            self._upload_thread_pool = ThreadPool(1)
 
         for remote, refs in refs_by_remote.items():
+            self._upload_thread_pool = ThreadPool(8 if parallel_upload else 1)
+
             self._output.info("Uploading to remote '{}':".format(remote.name))
 
             def upload_ref(ref_conanfile_prefs):
diff --git a/conans/test/functional/remote/multi_remote_checks_test.py b/conans/test/functional/remote/multi_remote_checks_test.py
index 85e2f36e334..41eb04787f8 100644
--- a/conans/test/functional/remote/multi_remote_checks_test.py
+++ b/conans/test/functional/remote/multi_remote_checks_test.py
@@ -79,6 +79,22 @@ def package_info(self):
         client.run("remote list_ref")
         self.assertIn("Pkg/0.1@lasote/testing: server3", client.out)
 
+    def test_multiple_remotes_single_upload(self):
+        servers = OrderedDict([("server1", TestServer()),
+                               ("server2", TestServer())])
+        client = TestClient(servers=servers, users={"server1": [("lasote", "mypass")],
+                                                    "server2": [("lasote", "mypass")]})
+        conanfile = """from conans import ConanFile
+class Pkg(ConanFile):
+    settings = "build_type"
+    """
+        client.save({"conanfile.py": conanfile})
+        client.run("create . Pkg/0.1@lasote/testing -s build_type=Release")
+        client.run("create . Pkg2/0.1@lasote/testing -s build_type=Release")
+        client.run("remote add_ref Pkg/0.1@lasote/testing server1")
+        client.run("remote add_ref Pkg2/0.1@lasote/testing server2")
+        client.run("upload Pkg* --all --confirm")
+
     def test_binary_packages_mixed(self):
         servers = OrderedDict([("server1", TestServer()),
                                ("server2", TestServer()),


Code review comments:
diff_hunk: @@ -201,12 +201,11 @@ def upload(self, reference_or_pattern, remotes, upload_recorder, package_id=None
                                            all_packages, query)
 
         if parallel_upload:
-            self._upload_thread_pool = ThreadPool(8)
             self._user_io.disable_input()
-        else:
-            self._upload_thread_pool = ThreadPool(1)
 
         for remote, refs in refs_by_remote.items():
+            self._upload_thread_pool = ThreadPool(8 if parallel_upload else 1)
path: conans/client/cmd/uploader.py
text: @author:
Maybe this should use tools.cpu_count()?

@user1:
This doesn't look right either, as the ``self._upload_thread_pool`` variable will be overwritten, losing access to the other pool from the previous remote.

@author:
Each iteration of this loop will complete before the next starts, since map() will block, then it will close the thread pool and start the next iteration.

@author:
Wait, we can definitely reuse the thread pool here. Keeping it alive the whole time makes a lot more sense.