Enabled Event and CapacityLimiter to be instantiated outside an event loop #651

agronholm · 2023-12-12T21:29:37Z

Relates to #500.

… loop

gschaffner · 2023-12-13T03:08:19Z

is the motivation for this #500 (comment) / #501 (comment)? if so, @tapetersen's example code has been working as intended since #524 was merged, so a delayed instantiation trick may not be necessary. see also #523 (comment) / python-trio/sniffio#35 (comment).

agronholm · 2023-12-13T10:23:16Z

The motivation is to allow users to create events and capacity limiters on the module level, as import side effects, and in the REPL too. You can already do this with asyncio.Event() and trio.Event().

davidbrochart · 2023-12-13T10:40:16Z

src/anyio/_core/_synchronization.py

+        self._event.set()
+
+    def is_set(self) -> bool:
+        return self._internal_event is not None and self._internal_event.is_set()


I understand that if the event is not created then it cannot be set, but should is_set actually create the event?

Suggested change

return self._internal_event is not None and self._internal_event.is_set()

return self._event.is_set()

statistics also doesn't need the event to be created (it could return empty statistics if it's not), yet it creates it.

perhaps statistics should have a special case (like is_set does) to allow lookup from outside of the event loop context?

Lock.statistics() for instance is accessible from outside of the event loop already.

davidbrochart · 2023-12-13T10:50:00Z

src/anyio/_core/_synchronization.py

+    def __new__(cls, total_tokens: int) -> CapacityLimiterAdapter:
+        return object.__new__(cls)
+
+    def __init__(self, total_tokens: int) -> None:


Line 519 checks if total_tokens is not math.inf which is a float, so should this be reflected in its type?

Suggested change

def __init__(self, total_tokens: int) -> None:

def __init__(self, total_tokens: int | float) -> None:

In that case, the annotation should be float, as that already includes integers according to the PEP 484 numeric tower.

Well, on the other hand it shouldn't be any float. Only math.inf actually makes sense.

Shouldn't int | math.inf Just Work?

No, because math.inf is not a type.

Neither is None, but that works just fine.

Ugh. They should just have accepted arbitrary singletons. (I do wonder whether any type checkers do …)

I think it should be int | Literal[math.inf], but mypy doesn't like it. Maybe it's a bug in mypy?

No, it's not. Floats are not valid literals according to PEP 586.

Thanks.
BTW, why not using None to mean an infinite number of tokens? math.inf carries a direct meaning, but None also means "I don't want to specify a finite number of tokens", which easily translates to an infinite number of tokens IMO.

It couldn't been done, but I was following Trio's API design which uses math.inf instead.

gschaffner · 2023-12-13T10:56:19Z

src/anyio/_core/_synchronization.py

@@ -76,7 +77,7 @@ class SemaphoreStatistics:

 class Event:
    def __new__(cls) -> Event:
-        return get_async_backend().create_event()
+        return EventAdapter()


would it be cheaper to avoid using the proxy in the common case?

def __new__(cls) -> Event: try: return get_async_backend().create_event() except sniffio.AsyncLibraryNotFoundError: return EventAdapter()

for the common case (construction under the loop context) this would move the overhead from on-every-attribute-access to on-construction.

gschaffner · 2023-12-13T10:56:23Z

src/anyio/_core/_synchronization.py

@@ -76,7 +77,7 @@ class SemaphoreStatistics:

 class Event:
    def __new__(cls) -> Event:
-        return get_async_backend().create_event()
+        return EventAdapter()


in the async methods of high-frequency primitives that construct Events (Lock.acquire, Semaphore.acquire, Condition.wait, MemoryObjectReceiveStream.receive, MemoryObjectSendStream.send) would it be good to call get_async_backend().create_event() directly to avoid the overhead from adapters?

With my latest change, they won't be getting the adapters, will they?

yep, implementing #651 (comment) addressed this comment too.

(technically Event() still has some overhead (get_async_backend().create_event() vs. try: get_async_backend().create_event()) but it should be very cheap https://docs.python.org/3/faq/design.html#how-fast-are-exceptions.)

gschaffner · 2023-12-13T11:01:50Z

src/anyio/_core/_synchronization.py

+        self._event.set()
+
+    def is_set(self) -> bool:
+        return self._internal_event is not None and self._internal_event.is_set()


perhaps statistics should have a special case (like is_set does) to allow lookup from outside of the event loop context?

Lock.statistics() for instance is accessible from outside of the event loop already.

gschaffner · 2023-12-13T11:19:31Z

tests/test_synchronization.py

@@ -208,6 +210,18 @@ async def waiter() -> None:

        assert event.statistics().tasks_waiting == 0

+    def test_instantiate_outside_event_loop(


to prevent the other synchronization primitives from regressing on this issue, it could make sense to add an analogous test_instantiate_outside_event_loop for each of them.

gschaffner

one minor comment, at your discretion. otherwise LGTM!

gschaffner · 2023-12-14T08:54:39Z

src/anyio/_core/_synchronization.py

@@ -76,7 +77,7 @@ class SemaphoreStatistics:

 class Event:
    def __new__(cls) -> Event:
-        return get_async_backend().create_event()
+        return EventAdapter()


yep, implementing #651 (comment) addressed this comment too.

(technically Event() still has some overhead (get_async_backend().create_event() vs. try: get_async_backend().create_event()) but it should be very cheap https://docs.python.org/3/faq/design.html#how-fast-are-exceptions.)

gschaffner · 2023-12-14T09:15:24Z

src/anyio/_core/_synchronization.py

+
+    @total_tokens.setter
+    def total_tokens(self, value: float) -> None:
+        self._limiter.total_tokens = value


(obscure/minor comment) could this be delayed too?

Suggested change

self._limiter.total_tokens = value

if self._internal_limiter is None:

self._total_tokens = value

else:

self._limiter.total_tokens = value

mhm, i suppose that if total_tokens.setter is delayed then this adapter might not be thread-safe. is it intended to be thread-safe?

Frankly, the idea that someone would change the adapter's total tokens from a worker thread had not even occurred to me. The whole idea seems pretty bizarre to me.

yeah, i think that it's reasonable to require them to do the mutation in the event loop thread. CancelScope.cancel for example is not safe to call from a non-main thread or a signal handler. Event.set is not thread-safe either. it's extra not thread safe after adding EventAdapter, but it wasn't thread-safe before either.

I'll revert my latest commit then?

The more I think about it, the more I'm leaning towards allowing users to mutate total_tokens before the initialization.

huh, does Trio document that?

i'm peeking at the Trio code and total_tokens.setter looks thread-unsafe—there's no threading.Lock, so if two threads mutate total_tokens in parallel then two _wake_waiters will run, which can cause too many waiters to get woken up.

(anyio.CapacityLimiter.)total_tokens.setter is currently thread-unsafe, though, so i think that d8ba6de is in alignment with the status quo.

The more I think about it, the more I'm leaning towards allowing users to mutate total_tokens before the initialization.

i'm leaning this way too.

Yeah, generally the API should be considered thread-unsafe, save for that parts that were explicitly designed to allow access from worker threads (the anyio.from_thread module).

oops—i misinterpreted

It might be reasonable to allow the tokens to be set from outside the event loop, just like trio.CapacityLimiter allows.

as

It might be reasonable to allow the tokens to be set from other threads, just like trio.CapacityLimiter allows.

and was replying to something you didn't say. :p

agronholm · 2023-12-14T10:04:20Z

Thank you for the reviews!

tapetersen · 2023-12-14T12:07:58Z

is the motivation for this #500 (comment) / #501 (comment)? if so, @tapetersen's example code has been working as intended since #524 was merged, so a delayed instantiation trick may not be necessary. see also #523 (comment) / python-trio/sniffio#35 (comment).

Oh i unfortunately hadn't noticed before. Like mentioned it wasn't really hard to workaround in our case. Nice to see it getting solved outside a loop at all as well as that was the case of asyncio and why I was surprised it didn't work.

Enabled Event and CapacityLimiter to be instantiated outside an event…

7c5bf4f

… loop

graingert approved these changes Dec 12, 2023

View reviewed changes

davidbrochart reviewed Dec 13, 2023

View reviewed changes

gschaffner reviewed Dec 13, 2023

View reviewed changes

agronholm added 7 commits December 13, 2023 13:30

Corrected type annotations for CapacityLimiter

9f9c811

Only instantiate the adapters if there is no current event loop

0dae529

Allowed more properties to be used when there is no running event loop

3db1677

Added tests for use outside event loop for other primitives

08c1d7d

Merge branch 'master' into lazy-event-instantiation

7db73e7

Merge branch 'master' into lazy-event-instantiation

d50165a

Merge branch 'master' into lazy-event-instantiation

f214590

gschaffner approved these changes Dec 14, 2023

View reviewed changes

Allowed total_tokens to be modified outside the event loop

d8ba6de

agronholm merged commit 28516e2 into master Dec 14, 2023
16 checks passed

agronholm deleted the lazy-event-instantiation branch December 14, 2023 10:04

	return self._internal_event is not None and self._internal_event.is_set()
	return self._event.is_set()

	def __init__(self, total_tokens: int) -> None:
	def __init__(self, total_tokens: int \| float) -> None:

		@@ -208,6 +210,18 @@ async def waiter() -> None:

		assert event.statistics().tasks_waiting == 0

		def test_instantiate_outside_event_loop(

Enabled Event and CapacityLimiter to be instantiated outside an event loop #651

Enabled Event and CapacityLimiter to be instantiated outside an event loop #651

Conversation

agronholm commented Dec 12, 2023

gschaffner commented Dec 13, 2023

agronholm commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smurfix Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

agronholm Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschaffner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschaffner Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschaffner Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschaffner Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm commented Dec 14, 2023

tapetersen commented Dec 14, 2023

smurfix Dec 13, 2023 •

edited

Loading

davidbrochart Dec 13, 2023 •

edited

Loading

agronholm Dec 13, 2023 •

edited

Loading

gschaffner Dec 14, 2023 •

edited

Loading

gschaffner Dec 14, 2023 •

edited

Loading

gschaffner Dec 14, 2023 •

edited

Loading