Delayed messages #102

fjarri · 2020-11-02T07:10:10Z

This PR implements delayed message delivery, useful for creating event loops which can still be terminated at a moment's notice. Possibly fixes #44. Also fixes an unrelated bug in eventlet.Event that caused event.wait() to result in an AttributeError.

Supporting delay in ask/tell is pretty straightforward, but doing it for proxy() requires some additional indirection. It is in the commit by itself now, because I understand that the approach I used may be questionable; I also hasn't changed the docs/tests because there is a probability the interface will be changed. Currently one would use delayed proxies as

proxy = actor_ref.proxy(extended=True)
proxy.delayed(1).something()
proxy.delayed(2).some.attr = 1

All the introspection is only done once when proxy_base is created.

This PR has some intersection of functionality with PR #95, but is simpler and gives actors more control of the process (specifically, it is more convenient for event loop organization).

codecov · 2020-11-02T20:12:17Z

Codecov Report

Merging #102 (50dac57) into main (ac006f7) will decrease coverage by 2.68%.
The diff coverage is 84.96%.

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   94.59%   91.91%   -2.69%     
==========================================
  Files          12       12              
  Lines         481      581     +100     
==========================================
+ Hits          455      534      +79     
- Misses         26       47      +21

Impacted Files	Coverage Δ
src/pykka/_proxy.py	`88.31% <77.33%> (-10.70%)`	⬇️
src/pykka/_ref.py	`98.11% <88.88%> (-1.89%)`	⬇️
src/pykka/_actor.py	`93.90% <95.12%> (-0.46%)`	⬇️
src/pykka/_envelope.py	`100.00% <100.00%> (ø)`
src/pykka/_threading.py	`96.42% <100.00%> (+0.20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac006f7...50dac57. Read the comment docs.

fjarri · 2020-11-05T07:43:52Z

@jodal the PR is ready for the initial review now

djmattyg007 · 2021-01-05T09:58:47Z

My initial thought upon reading the patch is that monotonic timers should be used. Right now it looks like a collision course for problems around daylight saving time changes and leap seconds.

fjarri · 2021-01-05T16:44:57Z

Isn't system timer always monotonic and not affected by timezones/daylight savings etc?

djmattyg007 · 2021-01-05T21:21:14Z

If that was true there wouldn't be any point for time.monotonic() to exist. The output of time.time() abaolutely is not guaranteed to be monotonic.

fjarri · 2021-01-05T21:31:04Z

My bad, I thought time.time() takes values from the system timer. Seems like an easy fix then?

djmattyg007 · 2021-01-05T21:35:11Z

I should have looked this up initially:

https://docs.python.org/3/library/time.html?highlight=monotonic#time.time

While this function normally returns non-decreasing values, it can return a lower value than a previous call if the system clock has been set back between the two calls.

You definitely want time.monotonic(). I'm not sure how difficult it would be to switch everything over, but hopefully in most cases if all you're doing is diffing timestamps it should be fine.

fjarri · 2021-01-09T04:52:48Z

@djmattyg007 , updated the PR, thanks for pointing out this problem. It was pretty easy, there were just two places where time.time() was used.

adamcik

Some of these comments are more nits and open questions. I would not run off and try and address everything/anything I pointed out but perhaps see what @jodal thinks about these before spending time on them.

Perhaps doing a wider documentation pass explaining the architecture and how things fit together makes sense after this is merged?

adamcik · 2021-03-07T06:43:25Z

src/pykka/_actor.py

+        return len(self.timestamps) == 0
+
+    def add(self, envelope):
+        idx = bisect.bisect(self.timestamps, envelope.timestamp)


Could this use heapq for a priority queue instead of using bisect like this? If there is a reason for preferring it as I would add it as a comment to help future readers og the code.

That's a good idea, I admit I wasn't aware of heapq's existence.

adamcik · 2021-03-07T06:47:45Z

src/pykka/_actor.py

+        if len(self.timestamps) == 0:
+            return None
+        else:
+            return max(self.timestamps[0] - time.monotonic(), 0)


Being able to sub in a different time source could be useful for tests. Probably not required for this initial PR, but worth keeping in mind.

Not sure what kind of API is needed for that, given that it has to match the call in _envelope.py. Perhaps it will be easier to just mock time.monotonic in tests?

You might want to read up on the clock pattern:
https://agilewarrior.wordpress.com/2017/03/03/clock-pattern/

So how is that different from mocking time.monotonic?

Mocking time.monotonic() in particular may not be feasible. A dedicated clock object can provide more flexibility in this regard, as well as a nicer interface to manage the clock in tests.

I guess that's true. But there's still a problem of similarly affecting the timeouted methods of Queue or Event.

adamcik · 2021-03-07T06:48:51Z

src/pykka/_actor.py

@@ -98,6 +124,11 @@ def _create_actor_inbox():
        """Internal method for implementors of new actor types."""
        raise NotImplementedError("Use a subclass of Actor")

+    @staticmethod
+    def _queue_empty_exception():
+        """Internal method for implementors of new actor types."""


Does this return a type or an instance, could we document this?

adamcik · 2021-03-07T06:51:31Z

src/pykka/_actor.py

+            # Take all the messages out of the inbox and put them
+            # in our internal inbox where they're sorted by timestamps
+            try:
+                envelope = self.actor_inbox.get(timeout=next_event_in)


What happens when timeout=0

The same as with any other timeout, really. It either returns an Envelope (if there's one in the queue), or goes into the except self.__queue_empty_exception branch.

adamcik · 2021-03-07T06:54:12Z

src/pykka/_actor.py

+                next_event_in = self.__timed_inbox.next_event_in()
+
+            # Check if there's something to be processed right now.
+            if next_event_in > 0:


I thought next event in can be zero? Is the check here so that we skip any future messages that have delays left and only run those that are ready. Perhaps makes this more clear with a comment?

Rather, if there are no messages to execute right now (that is, we received some messages, but they were all delayed), we go into the waiting mode again. If there is something to execute, next_event_in would be 0. I'm not sure how can I make it clearer in the comment; could you propose an alternative?

adamcik · 2021-03-07T07:15:02Z

src/pykka/_proxy.py

+    def __setattr__(self, name, value):
+        if name == "actor_ref" or name.startswith("_"):
+            return super().__setattr__(name, value)
+        message_factory = self._message_builder.setattr(name)


Reading this line I almost assumed it was changing an attr on the builder, but I assume it's creating a builder that has the job of setting an attr? Could we make this more clear with better naming or comments?

adamcik · 2021-03-07T07:15:57Z

src/pykka/_proxy.py

@@ -263,9 +343,10 @@ class CallableProxy:
        proxy.do_work.defer()
    """

-    def __init__(self, actor_ref, attr_path):
+    def __init__(self, actor_ref, attr_path, delay=0):


Type, units and documentation for delay?

adamcik · 2021-03-07T07:16:44Z

src/pykka/_ref.py


-    def ask(self, message, block=True, timeout=None):
+    def ask(self, message, block=True, timeout=None, delay=0):


Document, type, units and semantics of delay in ask?

Also should we allow this to be called with None and is that treated the same as zero? Or a negative delay? Or a duration from datetime?

Should we support an at=None API passing in a time?

Also should we allow this to be called with None and is that treated the same as zero?

That's possible.

Or a negative delay?

That already works, although the resulting effect is that the message effectively gets a higher priority (because it will be put at the start of the TimedInbox). Not sure if that's a useful thing to allow or not.

Or a duration from datetime?

Not sure what you mean here.

Should we support an at=None API passing in a time?

That's kinda dangerous in view of the earlier discussion about time.monotonic(). We'll have to translate at into the monotonic time, and then the user adjusts the clock, or daylight savings happen, and the execution time will change.

adamcik · 2021-03-07T07:25:21Z

tests/test_actor.py

+    actor_ref.tell(
+        {"command": "callback", "callback": lambda: lst.append(1)}, delay=0.5
+    )
+    event_set = event.wait()


Should this have some largish timeout just in case, so the test exists even if this breaks at some point?

adamcik · 2021-03-07T07:25:41Z

tests/test_actor.py

+        },
+        delay=delay,
+    )
+    event_set = event.wait()


Should this have some largish timeout just in case, so the test exists even if this breaks at some point?

fjarri · 2021-03-07T19:01:47Z

@adamcik , thanks for the comments. I was postponing writing the docs until I'm sure @jodal is happy with the API, especially that of ActorProxy (and with the PR in general).

fjarri force-pushed the delayed-delivery branch 3 times, most recently from 2890e7d to 29072ed Compare November 5, 2020 07:41

fjarri changed the title ~~[WIP] Delayed messages~~ Delayed messages Nov 5, 2020

fjarri force-pushed the delayed-delivery branch from 29072ed to 190998f Compare January 9, 2021 04:51

fjarri force-pushed the delayed-delivery branch from 190998f to 7659f5c Compare January 9, 2021 04:53

Base automatically changed from develop to main March 6, 2021 18:16

jodal force-pushed the delayed-delivery branch from 7659f5c to 214e7f1 Compare March 6, 2021 23:50

Delayed ask()/tell()

7b376d1

jodal force-pushed the delayed-delivery branch 2 times, most recently from ac7574e to b9c16d0 Compare March 6, 2021 23:55

Delayed proxy objects

50dac57

jodal force-pushed the delayed-delivery branch from b9c16d0 to 50dac57 Compare March 6, 2021 23:56

adamcik reviewed Mar 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delayed messages #102

Delayed messages #102

fjarri commented Nov 2, 2020 •

edited

Loading

codecov bot commented Nov 2, 2020 •

edited

Loading

fjarri commented Nov 5, 2020

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 5, 2021

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 5, 2021

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 9, 2021

adamcik left a comment

adamcik Mar 7, 2021

fjarri Mar 7, 2021

adamcik Mar 7, 2021

fjarri Mar 7, 2021

djmattyg007 Mar 7, 2021

fjarri Mar 7, 2021

djmattyg007 Mar 7, 2021

fjarri Mar 7, 2021

adamcik Mar 7, 2021

adamcik Mar 7, 2021

fjarri Mar 7, 2021 •

edited

Loading

adamcik Mar 7, 2021

fjarri Mar 7, 2021

adamcik Mar 7, 2021

adamcik Mar 7, 2021

adamcik Mar 7, 2021

adamcik Mar 7, 2021

fjarri Mar 7, 2021

adamcik Mar 7, 2021

adamcik Mar 7, 2021

fjarri commented Mar 7, 2021


		def ask(self, message, block=True, timeout=None):
		def ask(self, message, block=True, timeout=None, delay=0):

Delayed messages #102

Are you sure you want to change the base?

Delayed messages #102

Conversation

fjarri commented Nov 2, 2020 • edited Loading

codecov bot commented Nov 2, 2020 • edited Loading

Codecov Report

fjarri commented Nov 5, 2020

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 5, 2021

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 5, 2021

djmattyg007 commented Jan 5, 2021

fjarri commented Jan 9, 2021

adamcik left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjarri Mar 7, 2021 • 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

fjarri commented Mar 7, 2021

fjarri commented Nov 2, 2020 •

edited

Loading

codecov bot commented Nov 2, 2020 •

edited

Loading

fjarri Mar 7, 2021 •

edited

Loading