Enabling Event.set to notify all waiters with an exception
Morning guys, I came across with that idea trying to resolve a typical dogpile pattern [1], having many DNS calls to the same domain because of a miss in a DNS cache. The usage of the the set either to notify that the waiters could be awake and get the result from the cache or use it to notify that something was wrong helped me to reduce the complexity of the code. Just as an example: ``` if key in throttle_dns_events: yield from throttle_dns_events[key].wait() else: throttle_dns_events[key] = Event(loop=loop) try: addrs = yield from \ resolver.resolve(host, port, family=family) cached_hosts.add(key, addrs) throttle_dns_events[key].set() except Exception as e: # any DNS exception, independently of the implementation # is set for the waiters to raise the same exception. throttle_dns_events[key].set(exc=e) raise finally: throttle_dns_events.pop(key) ``` Any error caught by the locker will be broadcasted to the waiters. For example, a invalid hostname. I tried to open a PR to the CPython implementation, and they claim that the current interface of all of the locks objects behind the asyncio.locks [2] module try to keep the same interface as the threading one [3]. Therefore, to modify the asyncio implementation would need first a change in the threading interface. I was determined to justify that change, but after a bit research, I didn't find any example in other languages such as Java [4], C# [5] or C++ [6] allowing you to send an exception as a signal value to wake up the sleeping threads. is that enough to give up? I'm still reticent, I believe that this simple change in the interface can help reducing the complexity to handle errors in some scenarios. I would like to gather more ideas, thoughts, and comments from you about how can I still justify this change ... Thanks, [1] https://github.com/pfreixes/aiohttp/blob/throttle_dns_requests/aiohttp/conne... [2] https://docs.python.org/3/library/asyncio-sync.html [3] https://docs.python.org/3/library/threading.html [4] https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#notifyAll() [5] https://msdn.microsoft.com/en-us/library/system.threading.monitor.pulseall.a... [6] http://en.cppreference.com/w/cpp/thread/condition_variable/notify_all -- --pau
On Tue, Jul 18, 2017 at 2:33 PM, Pau Freixes <pfreixes@gmail.com> wrote:
Morning guys,
(Not everyone here is a guy.)
'Event' is designed as a lowish-level primitive: the idea is that it purely provides the operation of "waiting for something", and then you can compose it with other data structures to build whatever higher-level semantics you need. From this point of view, it doesn't make much sense to add features like exception throwing -- that would make it more useful for some particular cases, but add overhead that others don't want or need. In this case, don't you want to cache an error return as well, anyway? It sounds like you're reinventing the idea of a Future, which is intended as a multi-reader eventually-arriving value-or-error -- exactly what you want here. So it seems like you could just write: # Conceptually correct, but subtly broken due to asyncio quirks if key not in cache: cache[key] = asyncio.ensure_future(resolver.resolve(...)) return await cache[key] BUT, unfortunately, this turns out to be really broken when combined with asyncio's cancellation feature, so you shouldn't do this :-(. When using asyncio, you basically need to make sure to never await any given Future more than once. Maybe adding a shield() inside the await is the right solution? The downside is that you actually do want to propagate the cancellation into the resolution Task, just... only if *all* the callers are cancelled *and* only if you can make sure that the cancellation is not cached. It's quite tricky actually! But I don't think adding exception-throwing functionality to Event() is the right solution :-) -n -- Nathaniel J. Smith -- https://vorpus.org
Yeps,
I do agree, indeed this was the main rationale that I wrote down. All languages kept the same interface, Python shouldn't be different in this. But, in the other side, the change added to the set method is almost negligible, enabling as advantage reduce the complexity in the client code to handle some situations. Just that, pros and cons.
In this case, don't you want to cache an error return as well, anyway?
Not at all, the error is ephemeral, is never cached. If an error is produced, perhaps a network outage, this is broadcasted to the other waiting clients. Once there are no more waiting clients, the same DNS resolution will have the chance to make the proper query. No error caching.
Not at all, the idea is taking into advantage the Event principle, having a set of Futures waiting to be awakened and returning either a value or exception.
I've realized that I must protect the resolve() with a shield() for the caller that holds the event. Otherwise, the other waiters will have the chance to get a Canceled exception. Regarding the propagation of the cancellation if and only if *all* callers are canceled IMHO will fall on the side of a complex problem, and the solution might be do nothing.
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() -- --pau
On Wed, Jul 19, 2017 at 2:37 AM, Pau Freixes <pfreixes@gmail.com> wrote:
That's not the principle of Event. You are describing a Future. Regarding the propagation of the cancellation if and only if *all*
callers are canceled IMHO will fall on the side of a complex problem, and the solution might be do nothing.
If you're willing to do nothing when all callers cancel, then the Future solution that Nathaniel posted should work for you (replacing ensure_future() with shield()). Have you tried it? Do you have a specific objection to it?
On Jul 18, 2017 11:37 PM, "Pau Freixes" <pfreixes@gmail.com> wrote: Yeps,
[...]
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() Not really – the point of the first part of my message is that if you really want a Future/Event hybrid that can raise an error from 'wait', then python gives you the tools to implement this yourself, and then you can use it however you like. Something like class ErrorfulOneShotEvent: def __init__(self): self._event = asyncio.Event() self._error = None def set(self, error=None): self._error = error self._event.set() async def wait(self): await self._event.wait() if self._error is not None: raise self._error ...and you're good to go.
yes thats the behviour that I was looking for. The code its enought simple. Using the shield as has been proposed to protect the cancellation, plus this pattern it will work. Thanks for the feedback and proposal, I will abandon the idea of modify the set method. Cheers, El 19/07/2017 20:39, "Nathaniel Smith" <njs@pobox.com> escribió: On Jul 18, 2017 11:37 PM, "Pau Freixes" <pfreixes@gmail.com> wrote: Yeps,
[...]
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() Not really – the point of the first part of my message is that if you really want a Future/Event hybrid that can raise an error from 'wait', then python gives you the tools to implement this yourself, and then you can use it however you like. Something like class ErrorfulOneShotEvent: def __init__(self): self._event = asyncio.Event() self._error = None def set(self, error=None): self._error = error self._event.set() async def wait(self): await self._event.wait() if self._error is not None: raise self._error ...and you're good to go.
On Tue, Jul 18, 2017 at 2:33 PM, Pau Freixes <pfreixes@gmail.com> wrote:
Morning guys,
(Not everyone here is a guy.)
'Event' is designed as a lowish-level primitive: the idea is that it purely provides the operation of "waiting for something", and then you can compose it with other data structures to build whatever higher-level semantics you need. From this point of view, it doesn't make much sense to add features like exception throwing -- that would make it more useful for some particular cases, but add overhead that others don't want or need. In this case, don't you want to cache an error return as well, anyway? It sounds like you're reinventing the idea of a Future, which is intended as a multi-reader eventually-arriving value-or-error -- exactly what you want here. So it seems like you could just write: # Conceptually correct, but subtly broken due to asyncio quirks if key not in cache: cache[key] = asyncio.ensure_future(resolver.resolve(...)) return await cache[key] BUT, unfortunately, this turns out to be really broken when combined with asyncio's cancellation feature, so you shouldn't do this :-(. When using asyncio, you basically need to make sure to never await any given Future more than once. Maybe adding a shield() inside the await is the right solution? The downside is that you actually do want to propagate the cancellation into the resolution Task, just... only if *all* the callers are cancelled *and* only if you can make sure that the cancellation is not cached. It's quite tricky actually! But I don't think adding exception-throwing functionality to Event() is the right solution :-) -n -- Nathaniel J. Smith -- https://vorpus.org
Yeps,
I do agree, indeed this was the main rationale that I wrote down. All languages kept the same interface, Python shouldn't be different in this. But, in the other side, the change added to the set method is almost negligible, enabling as advantage reduce the complexity in the client code to handle some situations. Just that, pros and cons.
In this case, don't you want to cache an error return as well, anyway?
Not at all, the error is ephemeral, is never cached. If an error is produced, perhaps a network outage, this is broadcasted to the other waiting clients. Once there are no more waiting clients, the same DNS resolution will have the chance to make the proper query. No error caching.
Not at all, the idea is taking into advantage the Event principle, having a set of Futures waiting to be awakened and returning either a value or exception.
I've realized that I must protect the resolve() with a shield() for the caller that holds the event. Otherwise, the other waiters will have the chance to get a Canceled exception. Regarding the propagation of the cancellation if and only if *all* callers are canceled IMHO will fall on the side of a complex problem, and the solution might be do nothing.
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() -- --pau
On Wed, Jul 19, 2017 at 2:37 AM, Pau Freixes <pfreixes@gmail.com> wrote:
That's not the principle of Event. You are describing a Future. Regarding the propagation of the cancellation if and only if *all*
callers are canceled IMHO will fall on the side of a complex problem, and the solution might be do nothing.
If you're willing to do nothing when all callers cancel, then the Future solution that Nathaniel posted should work for you (replacing ensure_future() with shield()). Have you tried it? Do you have a specific objection to it?
On Jul 18, 2017 11:37 PM, "Pau Freixes" <pfreixes@gmail.com> wrote: Yeps,
[...]
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() Not really – the point of the first part of my message is that if you really want a Future/Event hybrid that can raise an error from 'wait', then python gives you the tools to implement this yourself, and then you can use it however you like. Something like class ErrorfulOneShotEvent: def __init__(self): self._event = asyncio.Event() self._error = None def set(self, error=None): self._error = error self._event.set() async def wait(self): await self._event.wait() if self._error is not None: raise self._error ...and you're good to go.
yes thats the behviour that I was looking for. The code its enought simple. Using the shield as has been proposed to protect the cancellation, plus this pattern it will work. Thanks for the feedback and proposal, I will abandon the idea of modify the set method. Cheers, El 19/07/2017 20:39, "Nathaniel Smith" <njs@pobox.com> escribió: On Jul 18, 2017 11:37 PM, "Pau Freixes" <pfreixes@gmail.com> wrote: Yeps,
[...]
But I don't think adding exception-throwing functionality to Event() is the right solution :-)
Then I will be forced to make the code stateful, getting as an output as a complex solution if you compare it with the code that you might get using the Event() Not really – the point of the first part of my message is that if you really want a Future/Event hybrid that can raise an error from 'wait', then python gives you the tools to implement this yourself, and then you can use it however you like. Something like class ErrorfulOneShotEvent: def __init__(self): self._event = asyncio.Event() self._error = None def set(self, error=None): self._error = error self._event.set() async def wait(self): await self._event.wait() if self._error is not None: raise self._error ...and you're good to go.
participants (3)
-
Mark E. Haase -
Nathaniel Smith -
Pau Freixes