Repository: Parquery/icontract (language: python)

Issue:
The `asyncio.gather` method only triggers the decorators to check the first item
# Overview
When using icontract.require and icontract.ensure (and maybe others that I haven't tested) with an async function and an async validator or a regular lambda, only the first coroutine in the gather is checked.

# Example Code

```python
import asyncio

import icontract

from random import randint
from typing import Union

from loguru import logger

async def is_between_0_and_100(value):
    sleep_time = randint(1, 3)
    logger.info('Function is_between_0_and_100 sleeping for {sleep_time}', sleep_time=sleep_time)
    await asyncio.sleep(sleep_time)
    return 0 <= value < 100


@icontract.require(lambda bar: is_between_0_and_100(bar), error=ValueError('bar must be between 0 and 100'))
@icontract.ensure(lambda result: result is True)
async def is_less_than_42(bar: Union[int, float]) -> bool:
    sleep_time = randint(1, 5)
    logger.info('Called with bar = {bar}. Sleeping for {s}s', bar=bar, s=sleep_time)
    await asyncio.sleep(sleep_time)
    logger.info('bar = {bar} Done sleeping', bar=bar)
    return bar < 42


async def main() -> None:
    result = await asyncio.gather(
        is_less_than_42(0),  # should be True and it is
        is_less_than_42(50.0),  # Should raise an icontract.ViolationError, but doesn't
        is_less_than_42(-1),  # Should raise a ValueError, but doesn't
        is_less_than_42(1000),  # Should raise a ValueError, but doesn't
    )

    print(f'{result = }')  # Should not print anything, prints [True, False, True, False]


asyncio.run(main())
```

# Expected Output
I would expect there to be either an iContract ViolationError or a ValueError depending on which of the methods with the violations finished first.

# Actual Output:

```shell
2023-01-17 16:02:53.557 | INFO     | __main__:is_between_0_and_100:12 - Function is_between_0_and_100 sleeping for 3
2023-01-17 16:02:53.557 | INFO     | __main__:is_less_than_42:21 - Called with bar = 50.0. Sleeping for 4s
2023-01-17 16:02:53.557 | INFO     | __main__:is_less_than_42:21 - Called with bar = -1. Sleeping for 2s
2023-01-17 16:02:53.557 | INFO     | __main__:is_less_than_42:21 - Called with bar = 1000. Sleeping for 2s
2023-01-17 16:02:55.558 | INFO     | __main__:is_less_than_42:23 - bar = -1 Done sleeping
2023-01-17 16:02:55.559 | INFO     | __main__:is_less_than_42:23 - bar = 1000 Done sleeping
2023-01-17 16:02:56.557 | INFO     | __main__:is_less_than_42:21 - Called with bar = 0. Sleeping for 4s
2023-01-17 16:02:57.558 | INFO     | __main__:is_less_than_42:23 - bar = 50.0 Done sleeping
2023-01-17 16:03:00.558 | INFO     | __main__:is_less_than_42:23 - bar = 0 Done sleeping
result = [True, False, True, False]
```

As you can see, there is only one time that the `is_between_0_and_100` method was called and that was for the first method in the gather.

# Note:
If I move the second function up above the first in the gather call, then I do end up with a ViolationError. Similarly, if I move the original 3rd or 4th method to the top, I get a ValueError.


Discussion / hints:
@mikelane thanks for reporting this! I'll have a look at it this or next week.
@mikelane I need to investigate more, but it seems the bug is due to using of `thread.local` instead of [ContextVar] at this spot:
https://github.com/Parquery/icontract/blob/e978860e3e744e16f982656967fd651192659bed/icontract/_checkers.py#L505

Namely, we need to track which functions are having their contracts in order to allow for recursive contract definitions (see [Recursion in Contracts]).

Please give me some more time to look deeper into this before I commit a mistake.

[ContextVar]: https://docs.python.org/3/library/contextvars.html#context-variables
[Recursion in Contracts]: https://icontract.readthedocs.io/en/latest/implementation_details.html#recursion-in-contracts

Fix patch:
diff --git a/icontract/_checkers.py b/icontract/_checkers.py
index 7b52039..8869cc1 100644
--- a/icontract/_checkers.py
+++ b/icontract/_checkers.py
@@ -1,9 +1,9 @@
 """Provide functions to add/find contract checkers."""
+import contextvars
 import functools
 import inspect
-import threading
 from typing import Callable, Any, Iterable, Optional, Tuple, List, Mapping, \
-    MutableMapping, Dict, cast
+    MutableMapping, Dict, cast, Set
 
 import icontract._represent
 from icontract._globals import CallableT, ClassT
@@ -502,7 +502,7 @@ def resolve_kwdefaults(sign: inspect.Signature) -> Dict[str, Any]:
 # contract checking is already in progress.
 #
 # The key refers to the id() of the function (preconditions and postconditions) or instance (invariants).
-_IN_PROGRESS = threading.local()
+_IN_PROGRESS = contextvars.ContextVar("_IN_PROGRESS", default=None)  # type: contextvars.ContextVar[Optional[Set[int]]]
 
 
 def decorate_with_checker(func: CallableT) -> CallableT:
@@ -533,7 +533,7 @@ def decorate_with_checker(func: CallableT) -> CallableT:
     # Determine the default argument values
     kwdefaults = resolve_kwdefaults(sign=sign)
 
-    id_func = str(id(func))
+    id_func = id(func)
 
     # (mristin, 2021-02-16)
     # Admittedly, this branching on sync/async is absolutely monstrous.
@@ -553,14 +553,23 @@ async def wrapper(*args, **kwargs):  # type: ignore
             if kwargs_error:
                 raise kwargs_error
 
+            # We need to create a new in-progress set if it is None as the ``ContextVar`` does not accept
+            # a factory function for the default argument. If we didn't do this, and simply set an empty
+            # set as the default, ``ContextVar`` would always point to the same set by copying the default
+            # by reference.
+            in_progress = _IN_PROGRESS.get()
+            if in_progress is None:
+                in_progress = set()
+                _IN_PROGRESS.set(in_progress)
+
             # Use try-finally instead of ExitStack for performance.
             try:
                 # If the wrapper is already checking the contracts for the wrapped function, avoid a recursive loop
                 # by skipping any subsequent contract checks for the same function.
-                if hasattr(_IN_PROGRESS, id_func):
+                if id_func in in_progress:
                     return await func(*args, **kwargs)
 
-                setattr(_IN_PROGRESS, id_func, True)
+                in_progress.add(id_func)
 
                 (preconditions, snapshots, postconditions) = _unpack_pre_snap_posts(wrapper)
 
@@ -596,8 +605,7 @@ async def wrapper(*args, **kwargs):  # type: ignore
 
                 return result
             finally:
-                if hasattr(_IN_PROGRESS, id_func):
-                    delattr(_IN_PROGRESS, id_func)
+                in_progress.discard(id_func)
     else:
 
         def wrapper(*args, **kwargs):  # type: ignore
@@ -606,14 +614,23 @@ def wrapper(*args, **kwargs):  # type: ignore
             if kwargs_error:
                 raise kwargs_error
 
+            # We need to create a new in-progress set if it is None as the ``ContextVar`` does not accept
+            # a factory function for the default argument. If we didn't do this, and simply set an empty
+            # set as the default, ``ContextVar`` would always point to the same set by copying the default
+            # by reference.
+            in_progress = _IN_PROGRESS.get()
+            if in_progress is None:
+                in_progress = set()
+                _IN_PROGRESS.set(in_progress)
+
             # Use try-finally instead of ExitStack for performance.
             try:
                 # If the wrapper is already checking the contracts for the wrapped function, avoid a recursive loop
                 # by skipping any subsequent contract checks for the same function.
-                if hasattr(_IN_PROGRESS, id_func):
+                if id_func in in_progress:
                     return func(*args, **kwargs)
 
-                setattr(_IN_PROGRESS, id_func, True)
+                in_progress.add(id_func)
 
                 (preconditions, snapshots, postconditions) = _unpack_pre_snap_posts(wrapper)
 
@@ -650,8 +667,7 @@ def wrapper(*args, **kwargs):  # type: ignore
 
                 return result
             finally:
-                if hasattr(_IN_PROGRESS, id_func):
-                    delattr(_IN_PROGRESS, id_func)
+                in_progress.discard(id_func)
 
     # Copy __doc__ and other properties so that doctests can run
     functools.update_wrapper(wrapper=wrapper, wrapped=func)
@@ -802,8 +818,18 @@ def wrapper(*args, **kwargs):  # type: ignore
                                     func, param_names, args, kwargs)) from err
 
             # We need to disable the invariants check during the constructor.
-            id_instance = str(id(instance))
-            setattr(_IN_PROGRESS, id_instance, True)
+
+            # We need to create a new in-progress set if it is None as the ``ContextVar`` does not accept
+            # a factory function for the default argument. If we didn't do this, and simply set an empty
+            # set as the default, ``ContextVar`` would always point to the same set by copying the default
+            # by reference.
+            in_progress = _IN_PROGRESS.get()
+            if in_progress is None:
+                in_progress = set()
+                _IN_PROGRESS.set(in_progress)
+
+            id_instance = id(instance)
+            in_progress.add(id_instance)
 
             # ExitStack is not used here due to performance.
             try:
@@ -814,8 +840,7 @@ def wrapper(*args, **kwargs):  # type: ignore
 
                 return result
             finally:
-                if hasattr(_IN_PROGRESS, id_instance):
-                    delattr(_IN_PROGRESS, id_instance)
+                in_progress.discard(id_instance)
 
     else:
         # (mristin, 2021-02-16)
@@ -840,11 +865,20 @@ async def wrapper(*args, **kwargs):  # type: ignore
                                     "the param names were {!r}, the args were {!r} and kwargs were {!r}").format(
                                         func, param_names, args, kwargs)) from err
 
+                # We need to create a new in-progress set if it is None as the ``ContextVar`` does not accept
+                # a factory function for the default argument. If we didn't do this, and simply set an empty
+                # set as the default, ``ContextVar`` would always point to the same set by copying the default
+                # by reference.
+                in_progress = _IN_PROGRESS.get()
+                if in_progress is None:
+                    in_progress = set()
+                    _IN_PROGRESS.set(in_progress)
+
                 # The following dunder indicates whether another invariant is currently being checked. If so,
                 # we need to suspend any further invariant check to avoid endless recursion.
-                id_instance = str(id(instance))
-                if not hasattr(_IN_PROGRESS, id_instance):
-                    setattr(_IN_PROGRESS, id_instance, True)
+                id_instance = id(instance)
+                if id_instance not in in_progress:
+                    in_progress.add(id_instance)
                 else:
                     # Do not check any invariants to avoid endless recursion.
                     return await func(*args, **kwargs)
@@ -861,8 +895,7 @@ async def wrapper(*args, **kwargs):  # type: ignore
 
                     return result
                 finally:
-                    if hasattr(_IN_PROGRESS, id_instance):
-                        delattr(_IN_PROGRESS, id_instance)
+                    in_progress.discard(id_instance)
 
         else:
 
@@ -877,9 +910,19 @@ def wrapper(*args, **kwargs):  # type: ignore
 
                 # The following dunder indicates whether another invariant is currently being checked. If so,
                 # we need to suspend any further invariant check to avoid endless recursion.
-                id_instance = str(id(instance))
-                if not hasattr(_IN_PROGRESS, id_instance):
-                    setattr(_IN_PROGRESS, id_instance, True)
+
+                # We need to create a new in-progress set if it is None as the ``ContextVar`` does not accept
+                # a factory function for the default argument. If we didn't do this, and simply set an empty
+                # set as the default, ``ContextVar`` would always point to the same set by copying the default
+                # by reference.
+                in_progress = _IN_PROGRESS.get()
+                if in_progress is None:
+                    in_progress = set()
+                    _IN_PROGRESS.set(in_progress)
+
+                id_instance = id(instance)
+                if id_instance not in in_progress:
+                    in_progress.add(id_instance)
                 else:
                     # Do not check any invariants to avoid endless recursion.
                     return func(*args, **kwargs)
@@ -896,8 +939,7 @@ def wrapper(*args, **kwargs):  # type: ignore
 
                     return result
                 finally:
-                    if hasattr(_IN_PROGRESS, id_instance):
-                        delattr(_IN_PROGRESS, id_instance)
+                    in_progress.discard(id_instance)
 
     functools.update_wrapper(wrapper=wrapper, wrapped=func)
 
diff --git a/precommit.py b/precommit.py
index bc5b729..4fa4072 100755
--- a/precommit.py
+++ b/precommit.py
@@ -86,6 +86,15 @@ def main() -> int:
         env=env)
     # yapf: enable
 
+    # yapf: disable
+    subprocess.check_call(
+        ["coverage", "run",
+         "--source", "icontract",
+         "-a", "-m", "tests_3_8.async.separately_test_concurrent"],
+        cwd=str(repo_root),
+        env=env)
+    # yapf: enable
+
     subprocess.check_call(["coverage", "report"])
 
     print("Doctesting...")


Test patch:
diff --git a/tests_3_8/async/separately_test_concurrent.py b/tests_3_8/async/separately_test_concurrent.py
new file mode 100644
index 0000000..66082bc
--- /dev/null
+++ b/tests_3_8/async/separately_test_concurrent.py
@@ -0,0 +1,49 @@
+# pylint: disable=missing-docstring
+# pylint: disable=no-self-use
+# pylint: disable=unnecessary-lambda
+# pylint: disable=disallowed-name
+import asyncio
+import random
+
+import icontract
+
+
+async def main() -> None:
+    # NOTE (mristin, 2023-01-26):
+    # This is a regression test for #255.
+    #
+    # We have to run it as a separate script since unittest.IsolatedAsyncioTestCase
+    # messes up the contextvars in such a way that they are shared among the coroutines.
+
+    async def is_between_0_and_100(value: int) -> bool:
+        sleep_time = random.randint(1, 3)
+        await asyncio.sleep(sleep_time)
+        return 0 <= value < 100
+
+    @icontract.require(
+        lambda bar: is_between_0_and_100(bar),
+        error=lambda bar: icontract.ViolationError(f"bar between 0 and 100, but got {bar}"))
+    async def is_less_than_42(bar: int) -> bool:
+        sleep_time = random.randint(1, 5)
+        await asyncio.sleep(sleep_time)
+        return bar < 42
+
+    results_or_errors = await asyncio.gather(
+        is_less_than_42(0),  # Should return True
+        is_less_than_42(101),  # Should violate the pre-condition
+        is_less_than_42(-1)  # Should violate the pre-condition
+        ,
+        return_exceptions=True)
+
+    assert len(results_or_errors) == 3
+    assert results_or_errors[0]
+
+    assert isinstance(results_or_errors[1], icontract.ViolationError)
+    assert "bar between 0 and 100, but got 101" == str(results_or_errors[1])
+
+    assert isinstance(results_or_errors[2], icontract.ViolationError)
+    assert "bar between 0 and 100, but got -1" == str(results_or_errors[2])
+
+
+if __name__ == "__main__":
+    asyncio.run(main())