Formalize endpoint running vs serving and cleanup some tests #75

pipermerriam · 2019-05-21T17:39:35Z

What was wrong?

This addresses a few loosely connected issues:

The name attribute on an endpoint was not reliably present.
Running and Serving were started using independent mechanisms but stopped using the same mechanism.
Some of the tests were a little indirect in the way they tested certain things.
The use of a queue for internal events seemed like un-necessary overhead.
No formal API for checking if an endpoint was runnind or serving.
No checks for if already running/serving
No check in core connect_to_endpoint API for whether already connected.

How was it fixed?

Endpoints now always have a name
Firmer distinction between running and serving start and stop methods
Tried to make tests more direct
Removed use of a Queue for internal events in favor of immediate processing.
Added check for whether already serving or running
Added check to connect_to_endpoint API for duplicates.

Cute Animal Picture

pipermerriam · 2019-05-21T17:56:19Z

None of my pull requests on this repo run the first time I push them. Anyone else experiencing that?

pipermerriam

I have a bunch of cleanup to do first...

pipermerriam · 2019-05-21T18:07:59Z

lahja/asyncio/endpoint.py

+        while self.is_running:
+            try:
+                (item, config) = await self._receiving_queue.get()
+            except RuntimeError as err:


TODO: this should get pulled out to it's own PR.

lahja/asyncio/endpoint.py


    def receive_message(self, message: Broadcast) -> None:
+        self.logger.info("RECEIVING: %s", message)


lahja/asyncio/endpoint.py

+        self._child_tasks.add(asyncio.ensure_future(self._connect_receiving_queue()))
+
+        await self._receiving_loop_running.wait()
+        self.logger.info("Endpoint %s started", self.name)


lahja/asyncio/endpoint.py

+                if str(err) == "Event loop is closed":
+                    break
+                raise
+            self.logger.info("GOT %s from receiving queue", item)


lahja/asyncio/endpoint.py

@@ -534,6 +527,7 @@ def is_connected_to(self, endpoint_name: str) -> bool:
        return endpoint_name in self._outbound_connections

    def _process_item(self, item: BaseEvent, config: Optional[BroadcastConfig]) -> None:
+        self.logger.info("PROCESSING: %s", item)


pipermerriam · 2019-05-21T18:13:19Z

lahja/base.py

@@ -70,7 +82,7 @@ def serve(cls, config: ConnectionConfig) -> AsyncContextManager["BaseEndpoint"]:
            # server stopped

        """
-        pass
+        ...


@carver @cburgdorf what do you think about this convention. It feels like it make abstractmethods more visually distinct.

I'm happy with it 👍 Although if we start using autoflake maybe it will be moot because either one (pass or ...) would be stripped out.

Looks like autoflake leaves ... in place so maybe that's an argument to use it.

Interesting 👍

In general, I'm:+1: but there is one thing I'd like to bring up.

Consider, that you forget to subclass from ABC like we recently had in Trinity

In that case the @abstractmethod is simply ignored and neither pass nor ... give you any safety here. The only pattern that still ends up doing the right thing in that situation is with raise.

from abc import ABC, abstractmethod class Foo(): @abstractmethod def bar(self): raise Exception("Needs to implemented in subclass") Foo().bar()

I'm not insisting we should go back to raising errors but I wanted to bring it up as food for thought.

lahja/base.py

        event = await agen.asend(None)
+        self.logger.info("CLOSING STREAM")


pipermerriam · 2019-05-21T18:14:14Z

tests/core/asyncio/test_broadcast.py

-
-import pytest
-
-from lahja import AsyncioEndpoint, BaseEvent, ConnectionConfig


These tests don't really belong here and shouldn't be introduced until the reverse connection APIs go into place.

pipermerriam · 2019-05-21T18:15:27Z

tests/core/asyncio/test_connect.py

-    await endpoint.start_serving(own)
-
-    with pytest.raises(ConnectionAttemptRejected):
-        endpoint.connect_to_endpoints_nowait(own, own)


These felt really odd to be testing using these non-core APIs so I changed to only use connect_to_endpoint.

I'm feeling like I want to remove the various extra connect_to_endpointXXXXX APIs as they feel like they add clutter and are only moderately more useful.

A quick scan through Trinity shows that we are still using connect_to_endpoints_nowait but it looks like the usage could be easily dropped now. I'm on board with removing connect_to_endpoints_nowait 👍

pipermerriam · 2019-05-21T18:16:13Z

tests/core/asyncio/test_stop.py

-
-    await second_endpoint.broadcast(DummyResponse(None))
-
-    second_endpoint.stop()


Deleted this test because it didn't feel valuable after writing a few tests for Endpoint.run() API.

pipermerriam · 2019-05-21T18:16:53Z

lahja/asyncio/endpoint.py

@@ -509,6 +501,7 @@ def connect_to_endpoints_nowait(self, *endpoints: ConnectionConfig) -> None:
        await self.connect_to_endpoint(endpoint)

    async def connect_to_endpoint(self, config: ConnectionConfig) -> None:
+        self._throw_if_already_connected(config)


This should be pulled out to a separate PR.

addressed in #77 but leaving in here so tests pass.

cburgdorf

Quite a few changes. Overall looks like a great improvement 👍
I left some comments that you might want to address but nothing big that stands out.

cburgdorf · 2019-05-22T11:27:19Z

lahja/asyncio/endpoint.py

@@ -593,7 +599,7 @@ def stop(self) -> None:
        if config is not None and config.internal:
            # Internal events simply bypass going through the central event bus
            # and are directly put into the local receiving queue instead.


The comment 👆 is still mentioning the "local receiving queue" that does no longer exist.

cburgdorf · 2019-05-22T11:37:45Z

lahja/base.py

@@ -70,7 +82,7 @@ def serve(cls, config: ConnectionConfig) -> AsyncContextManager["BaseEndpoint"]:
            # server stopped

        """
-        pass
+        ...


In general, I'm:+1: but there is one thing I'd like to bring up.

Consider, that you forget to subclass from ABC like we recently had in Trinity

In that case the @abstractmethod is simply ignored and neither pass nor ... give you any safety here. The only pattern that still ends up doing the right thing in that situation is with raise.

from abc import ABC, abstractmethod class Foo(): @abstractmethod def bar(self): raise Exception("Needs to implemented in subclass") Foo().bar()

I'm not insisting we should go back to raising errors but I wanted to bring it up as food for thought.

cburgdorf · 2019-05-22T11:50:06Z

tests/core/asyncio/test_basics.py

+)
+
+
+class Response(BaseEvent):


Why does this file declare new events instead of reusing those from helpers?

The helpers events were adding mental overhead. I had to remember if they had properties/constructor arguments, deal with importing them. I've also liked using semantically meaningful names in the tests which is less apparent here, but more-so in some of the other ones I've written.

And last, I have tried to move away from imports from modules within the ./tests namespace. They feel wrong. Nothing under ./tests are actually proper python modules (no __init__.py present), so I've been leaning towards relegating anything that needs to be re-used in tests to either be a fixture or live in library.tools somewhere.

cburgdorf · 2019-05-22T11:54:40Z

tests/core/asyncio/test_connect.py

-    await endpoint.start_serving(own)
-
-    with pytest.raises(ConnectionAttemptRejected):
-        endpoint.connect_to_endpoints_nowait(own, own)


A quick scan through Trinity shows that we are still using connect_to_endpoints_nowait but it looks like the usage could be easily dropped now. I'm on board with removing connect_to_endpoints_nowait 👍

cburgdorf · 2019-05-22T11:56:51Z

tests/core/asyncio/test_internal.py

+
+
+class Internal(BaseEvent):
+    pass


Why not use DummyRequest from helpers?

cburgdorf · 2019-05-22T12:02:54Z

tests/core/asyncio/test_internal.py

+    asyncio.ensure_future(do_wait_for(endpoint_a, got_by_endpoint_a))
+    will_not_finish = asyncio.ensure_future(do_wait_for(endpoint_b, got_by_endpoint_b))
+    # give subscriptions time to update
+    await asyncio.sleep(0.01)


This could be written as:

await wait_until_all_connections_subscribed_to(Internal)

cburgdorf · 2019-05-22T12:04:10Z

tests/core/asyncio/test_internal.py

+    await asyncio.sleep(0.01)
+
+    assert got_by_endpoint_a.is_set() is True
+    assert got_by_endpoint_b.is_set() is False


The test got a whole lot more readable 👍

pipermerriam force-pushed the piper/events-always-have-a-name branch from 6be67bf to 2420657 Compare May 21, 2019 17:56

pipermerriam requested a review from cburgdorf May 21, 2019 18:07

pipermerriam commented May 21, 2019

View reviewed changes

pipermerriam force-pushed the piper/events-always-have-a-name branch from 2420657 to 34a37bd Compare May 21, 2019 18:38

pipermerriam mentioned this pull request May 21, 2019

Make start/stop/start_server/stop_server private APIs #80

Open

pipermerriam force-pushed the piper/events-always-have-a-name branch from 34a37bd to 3c98105 Compare May 21, 2019 20:26

cburgdorf approved these changes May 22, 2019

View reviewed changes

Formalize endpoint running vs serving and cleanup some tests

336eca0

pipermerriam force-pushed the piper/events-always-have-a-name branch from 3c98105 to 336eca0 Compare May 23, 2019 13:25

PR feedback

49cc47f

pipermerriam merged commit 6d48a3b into ethereum:master May 23, 2019

pipermerriam deleted the piper/events-always-have-a-name branch May 23, 2019 14:33

pipermerriam mentioned this pull request May 24, 2019

AttributeError: 'Endpoint' object has no attribute '_name' #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formalize endpoint running vs serving and cleanup some tests #75

Formalize endpoint running vs serving and cleanup some tests #75

pipermerriam commented May 21, 2019

pipermerriam commented May 21, 2019

pipermerriam left a comment

pipermerriam May 21, 2019

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

pipermerriam May 21, 2019

carver May 21, 2019 •

edited

Loading

pipermerriam May 21, 2019

carver May 21, 2019

cburgdorf May 22, 2019

This comment was marked as resolved.

pipermerriam May 21, 2019

pipermerriam May 21, 2019

cburgdorf May 22, 2019

pipermerriam May 21, 2019

pipermerriam May 21, 2019

pipermerriam May 21, 2019

cburgdorf left a comment

cburgdorf May 22, 2019

cburgdorf May 22, 2019

cburgdorf May 22, 2019

pipermerriam May 23, 2019

cburgdorf May 22, 2019

cburgdorf May 22, 2019

cburgdorf May 22, 2019

cburgdorf May 22, 2019


		def receive_message(self, message: Broadcast) -> None:
		self.logger.info("RECEIVING: %s", message)

		event = await agen.asend(None)
		self.logger.info("CLOSING STREAM")


		import pytest

		from lahja import AsyncioEndpoint, BaseEvent, ConnectionConfig


		await second_endpoint.broadcast(DummyResponse(None))

		second_endpoint.stop()

Formalize endpoint running vs serving and cleanup some tests #75

Formalize endpoint running vs serving and cleanup some tests #75

Conversation

pipermerriam commented May 21, 2019

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam commented May 21, 2019

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

carver May 21, 2019 • 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

This comment was marked as resolved.

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

cburgdorf 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

carver May 21, 2019 •

edited

Loading