Repository: urllib3/urllib3

Issue:
Fix `RecentlyUsedContainer` regression
#2115 added typing to the `_collections` module. Unfortunately, it also introduced a regression that breaks `test_urllib3_pool_connection_closed` in the requests test suite, as noticed in https://github.com/urllib3/urllib3/pull/2316/checks?check_run_id=3049608329. `__setitem__` is implemented like this in 1.26.x:

https://github.com/urllib3/urllib3/blob/ba95e9eac73452d3bccfb5413b00d9a4fe3e4c31/src/urllib3/_collections.py#L61-L74

And it's now implemented like this in main:

https://github.com/urllib3/urllib3/blob/baba8c2b4aabda8907a777039baa3f8758e4333a/src/urllib3/_collections.py#L133-L155

When `len(self._container)` == `self._maxsize` == 0, `self._container[key] = value` is now called *after* `self._container.popitem(last=False)`, which is why that `popitem` call raises an exception.

Applying this hacky diff fixes the requests test:

```diff
diff --git a/src/urllib3/_collections.py b/src/urllib3/_collections.py
index 32d5330e..798ba432 100644
--- a/src/urllib3/_collections.py
+++ b/src/urllib3/_collections.py
@@ -139,15 +139,15 @@ class RecentlyUsedContainer(Generic[_KT, _VT], MutableMapping[_KT, _VT]):
                 # size of the pool. Because accessing a key should move it to
                 # the end of the eviction line, we pop it out first.
                 evicted_item = key, self._container.pop(key)
+                self._container[key] = value
             except KeyError:
+                self._container[key] = value
                 if len(self._container) >= self._maxsize:
                     # If we didn't evict an existing value, and we've hit our maximum
                     # size, then we have to evict the least recently used item from
                     # the beginning of the container.
                     evicted_item = self._container.popitem(last=False)
 
-            # Finally, insert the new value.
-            self._container[key] = value
 
         # After releasing the lock on the pool, dispose of any evicted value.
         if evicted_item is not None and self.dispose_func:
```

@haikuginger What are your thoughts on this? Is it possible to revert to the original logic?

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3) Report
> Merging [#2327](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3) (30cc819) into [main](https://codecov.io/gh/urllib3/urllib3/commit/baba8c2b4aabda8907a777039baa3f8758e4333a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3) (baba8c2) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/urllib3/urllib3/pull/2327/graphs/tree.svg?width=650&height=150&src=pr&token=UYKHB2uXDn&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3)](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3)

```diff
@@             Coverage Diff             @@
##              main    #2327      +/-   ##
===========================================
- Coverage   100.00%   99.63%   -0.37%     
===========================================
  Files           25       25              
  Lines         2456     2457       +1     
===========================================
- Hits          2456     2448       -8     
- Misses           0        9       +9     
```


| [Impacted Files](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3) | Coverage Δ | |
|---|---|---|
| [src/urllib3/\_collections.py](https://codecov.io/gh/urllib3/urllib3/pull/2327/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3#diff-c3JjL3VybGxpYjMvX2NvbGxlY3Rpb25zLnB5) | `99.43% <100.00%> (-0.57%)` | :arrow_down: |
| [src/urllib3/util/wait.py](https://codecov.io/gh/urllib3/urllib3/pull/2327/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3#diff-c3JjL3VybGxpYjMvdXRpbC93YWl0LnB5) | `91.11% <0.00%> (-8.89%)` | :arrow_down: |
| [src/urllib3/connection.py](https://codecov.io/gh/urllib3/urllib3/pull/2327/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3#diff-c3JjL3VybGxpYjMvY29ubmVjdGlvbi5weQ==) | `98.82% <0.00%> (-1.18%)` | :arrow_down: |
| [src/urllib3/response.py](https://codecov.io/gh/urllib3/urllib3/pull/2327/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3#diff-c3JjL3VybGxpYjMvcmVzcG9uc2UucHk=) | `99.50% <0.00%> (-0.50%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3). Last update [baba8c2...30cc819](https://codecov.io/gh/urllib3/urllib3/pull/2327?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=urllib3).


Patch:
diff --git a/src/urllib3/_collections.py b/src/urllib3/_collections.py
index 32d5330e..b0b0871d 100644
--- a/src/urllib3/_collections.py
+++ b/src/urllib3/_collections.py
@@ -139,16 +139,17 @@ class RecentlyUsedContainer(Generic[_KT, _VT], MutableMapping[_KT, _VT]):
                 # size of the pool. Because accessing a key should move it to
                 # the end of the eviction line, we pop it out first.
                 evicted_item = key, self._container.pop(key)
+                self._container[key] = value
             except KeyError:
-                if len(self._container) >= self._maxsize:
+                # When the key does not exist, we insert the value first so that
+                # evicting works in all cases, including when self._maxsize is 0
+                self._container[key] = value
+                if len(self._container) > self._maxsize:
                     # If we didn't evict an existing value, and we've hit our maximum
                     # size, then we have to evict the least recently used item from
                     # the beginning of the container.
                     evicted_item = self._container.popitem(last=False)
 
-            # Finally, insert the new value.
-            self._container[key] = value
-
         # After releasing the lock on the pool, dispose of any evicted value.
         if evicted_item is not None and self.dispose_func:
             _, evicted_value = evicted_item


Test patch:
diff --git a/test/test_collections.py b/test/test_collections.py
index 7cc1b868..14ead448 100644
--- a/test/test_collections.py
+++ b/test/test_collections.py
@@ -22,6 +22,11 @@ class TestLRUContainer:
         assert 0 not in d
         assert (i + 1) in d
 
+    def test_maxsize_0(self):
+        d = Container(0)
+        d[1] = 1
+        assert len(d) == 0
+
     def test_expire(self):
         d = Container(5)