Repository: huggingface/huggingface_hub
Pull request #1545 (2023-07-07 12:55:53)

Make `requests.Session` truly unique
Found a bug in `huggingface_hub` while investigating why CI in https://github.com/huggingface/lighteval-harness/pull/113 was failing (private repo). 

The triggered error was:
```
HTTPSConnectionPool(host='huggingface.co', port=443): Read timed out. (read timeout=10)' thrown while requesting HEAD https://huggingface.co/gpt2/resolve/main/config.json
```

It turns out that the error happens because a `request.Session` is shared between processes and somehow the session gets broken when forked. Sharing the session was not intended and is due to a bug in [`./utils/_http.py`](https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/utils/_http.py). To avoid sharing a session object, I was only checking the thread ID. This PR fixes the issue by checking the process ID as well. 

This PR fixes the lighteval repo CI and should also make `huggingface_hub` more stable (hopefully fix random ConnectionErrors like in https://github.com/huggingface/huggingface_hub/pull/1538 or https://github.com/huggingface/huggingface_hub/issues/1542).

cc @XciD @clefourrier @thomwolf 


Patch:
diff --git a/src/huggingface_hub/utils/_http.py b/src/huggingface_hub/utils/_http.py
index 013c94c9..0674ddc3 100644
--- a/src/huggingface_hub/utils/_http.py
+++ b/src/huggingface_hub/utils/_http.py
@@ -14,6 +14,7 @@
 # limitations under the License.
 """Contains utilities to handle HTTP requests in Huggingface Hub."""
 import io
+import os
 import threading
 import time
 import uuid
@@ -144,11 +145,11 @@ def get_session() -> requests.Session:
     session = get_session()
     ```
     """
-    return _get_session_from_cache(thread_ident=threading.get_ident())
+    return _get_session_from_cache(process_id=os.getpid(), thread_id=threading.get_ident())
 
 
 @lru_cache
-def _get_session_from_cache(thread_ident: int) -> requests.Session:
+def _get_session_from_cache(process_id: int, thread_id: int) -> requests.Session:
     """
     Create a new session per thread using global factory. Using LRU cache (maxsize 128) to avoid memory leaks when
     using thousands of threads. Cache is cleared when `configure_http_backend` is called.


Test patch:
diff --git a/tests/test_utils_http.py b/tests/test_utils_http.py
index ac9b3add..dc45aa17 100644
--- a/tests/test_utils_http.py
+++ b/tests/test_utils_http.py
@@ -1,6 +1,7 @@
 import threading
 import time
 import unittest
+from multiprocessing import Process, Queue
 from typing import Generator
 from unittest.mock import Mock, call, patch
 from uuid import UUID
@@ -213,6 +214,29 @@ class TestConfigureSession(unittest.TestCase):
                 if i != j:
                     self.assertIsNot(sessions[i], sessions[j])
 
+    def test_get_session_in_forked_process(self):
+        # Get main process session
+        main_session = get_session()
+
+        # Fork a new process and get session in it
+        Process(target=_process_target_get_session_in_forked_process).start()
+        child_session = process_queue.get()
+
+        # Check sessions are different
+        self.assertNotEqual(repr(main_session), child_session)
+
+
+# Used in test_get_session_in_forked_process
+# Must be defined at global scope to be pickable on Windows.
+# See https://stackoverflow.com/a/72776044.
+process_queue = Queue()
+
+
+def _process_target_get_session_in_forked_process():
+    # Put `repr(session)` in queue because putting the `Session` object directly would duplicate it.
+    # Repr looks like this: "<requests.sessions.Session object at 0x7f5adcc41e40>"
+    process_queue.put(repr(get_session()))
+
 
 class TestUniqueRequestId(unittest.TestCase):
     api_endpoint = ENDPOINT + "/api/tasks"  # any endpoint is fine