Exit longlived greenlets if any of them fails #2208

andrevmatos · 2018-08-17T09:48:53Z

Implements 3rd point of #2201

hackaugusto · 2018-08-17T16:01:33Z

raiden/network/transport/matrix.py

@@ -280,6 +280,11 @@ def stop_and_wait(self):
            self._client.greenlets.clear()
            self._client.logout()

+    @property
+    def greenlet(self) -> gevent.Greenlet:


I think this is a bad approach, why don't you make the RaidenService and Transport classes greenlets, and let them manage their child tasks instead of exposing it?

@hackaugusto I thought about it too, but these classes aren't greenlets on theirselves, as there's no obvious loop inside (MatrixTransport depends on the underlying _client.sync_thread greenlet, RaidenService doesn't actually have a loop/long-lasting _run method or alike, APIServer depends on the WSGIServer instance, which isn't even a greenlet on itself nor have one, but use low-level gevent calls to asynchronously dispatch request handling, that greenlet inside it just wraps the busy wait).
Rather, they control one or more subtasks/greenlets, like AlarmTask, GMatrixClient and WSGIServer, and are the responsible for starting/stopping them and being a point of control if the underlying task fails unexpectedly. How about a RaidenTask abstract class, with start, stop, possibly join methods and a long_greenlets() -> List[Greenlet] property? This would be only for these few long-lived tasks, as the other ones should fit nicely on the standard greenlet contract

An option would also to make a small StoppableGreenlet abstract class (greenlet with synchronous stop method), and make those classes inherint from it, making them effectivelly greenlets (as you asked), but in those cases, the _run method would be a simple get on the children tasks, which would make it re-raise underlying exceptions, if any, and cleanup and exit cleanly when stop is called

Yeah, having a standard method to stop all these classes is good, I just want to avoid the introspection used here:

raiden/raiden/ui/cli.py

Lines 1015 to 1025 in d8839ae

def stop_task(task, timeout=None):

try:

if callable(getattr(task, 'stop', None)):

task.stop()

finally:

if isinstance(task, gevent.Greenlet):

task.kill()

task.get(timeout=timeout)

elif hasattr(task, 'greenlet'):

task.greenlet.kill()

task.greenlet.get(timeout=timeout)

coveralls · 2018-08-21T14:39:25Z

Coverage increased (+24.5%) to 76.572% when pulling 8978fd4 on andrevmatos:exit_longlived_greenlets into b65d406 on raiden-network:master.

raiden/ui/cli.py

hackaugusto · 2018-08-23T14:30:05Z

raiden/tests/utils/network.py

@@ -313,7 +313,7 @@ def create_apps(
                                'server': 'matrix.local.raiden',
                            },
                            'retries_before_backoff': retries_before_backoff,
-                            'retry_interval': retry_interval,
+                            'retry_interval': retry_interval * 5,


this is not the right place for this change, please change it on raiden.tests.fixtures.variables

Problem is that that fixture doesn't have yet access to the transport. Will look on how to inject that there

hackaugusto · 2018-08-23T14:32:41Z

raiden/tasks.py

-        self.stop_event.set(True)
+        self._stop_event.set(True)
+
+    def stop(self, timeout=None):


please don't add timeouts for the inner layers, only use it at the outer-most layers of the code

It's just to comply with the interface. Inner layers may leave then as None as well

hackaugusto · 2018-08-23T14:34:25Z

raiden/raiden_service.py

                self.blockchain_events.uninstall_all_event_listeners()
        except (gevent.timeout.Timeout, RaidenShuttingDown):
-            pass
+            log.exception('Exception on RaidenService stop')


RaidenShuttingDownexceptions are expcted durring shutting down and should not be logged.

I hope RaidenShuttingDown won't be needed anymore after proper stop and exception handling here in this PR, but no hard feelings for it too

hackaugusto · 2018-08-23T14:36:24Z

raiden/network/rpc/client.py

@@ -237,9 +229,6 @@ def block_number(self):
        """ Return the most recent block. """
        return self.web3.eth.blockNumber

-    def inject_stop_event(self, event):


Why was this removed?

It's in the commit's comment: the correct approach both for exception handling and stop time should remove the need for injecting/raising an exception in web3. Instead, whatever uses it should stop and exit gracefully.

Okay, could you please remove the exception RaidenShuttingDown? This only exists because we were not relying on GreenletExit, and it is only raised from the web3 middleware.

hackaugusto · 2018-08-23T14:46:00Z

raiden/network/transport/matrix.py

+            for delay in timeout_generator:
+                state = self._address_to_presence.get(receiver_address)
+                if state in reachable:
+                    self._send_raw(receiver_address, data)


This is a blocking call, correct?

The if self._stop_event.wait(delay): bellow will not be sufficient to exit the task on a timely manner. Is the kill introduced in raiden-libs used to exit this?

Yes, _send_raw blocks, but should be very quick (only request send time), so why _stop_event.wait(delay) isn't enough? ~~The Timeout on stop waiting for these greenlets~~ timeout on shutdown should ensure it won't block forever

hackaugusto · 2018-08-23T14:48:20Z

raiden/network/transport/matrix.py

        else:
            self.log.error(
                'Invalid message',
                message=data,
            )

    def _receive_delivered(self, delivered: Delivered):
-        # FIXME: The signature doesn't seem to be verified - check in UDPTransport as well
+        # FIXME: check if UDPTransport also checks Delivered sender and message presence


The sender must be checked, otherwise a malicious node could forge a delivered message, which would clear the queue and stop retries.

This is only necessary because UDP is not an encrypted transport, so we need to authenticate using signatures.

Yes, I know.. the point is that I did that there for Matrix, so it's just updating the FIXME to remember to do this check also for UDP eventually.

hackaugusto · 2018-08-23T14:56:35Z

raiden/network/transport/matrix.py

-                )
-            discovery_room = self._client.create_room(self._discovery_room_alias, is_public=True)
+        last_ex = None
+        for _ in range(10):


These nested exceptions are hard to read, cant you split this in two loops?

while not room_exists and retry(): try: create_room() except: ... if not room_exists: raise while not joined and retry(): try: join() except: ... if not joined: raise

hackaugusto · 2018-08-23T14:57:35Z

raiden/network/transport/matrix.py

            return
+
+        message_id = getattr(message, 'message_identifier')


Please use message.message_identifier

hackaugusto · 2018-08-23T14:58:20Z

raiden/network/transport/matrix.py

+        self._client.set_presence_state(UserPresence.OFFLINE.value)
+
+        try:
+            with gevent.Timeout(timeout, 'Logging out despite unjoined greenlets.'):


we already have a timeout on the raiden service, no need for this here

All Runnable have the timeout parameter for stop, I'm just sticking to the interface/behavior, it was already there, just updated for the new interface.

Please don't add the timeout here, we have had multiple discussions about this and timeouts don't compose.

LefterisJP · 2018-08-25T13:11:21Z

You should rebase this on latest master. Lots of conflicting files @andrevmatos

andrevmatos · 2018-08-25T13:27:22Z

Yes, I know, but a lot of fixes which weren't directly related but needed for this PR were split on #2268, I may look into merging that first then this one on top of that. Hope to get that ready soon, only a single test failing in a non locally reproducible and weird way (gas exceeds limit error on teardown?)

raiden/api/rest.py

hackaugusto · 2018-08-31T10:17:20Z

raiden/utils/runnable.py

+    def __init__(self, run=None, *args, **kwargs):
+        if run is not None:
+            self.run = run
+        self.args = tuple(args)


why is this call necessary?

Just to ensure it's a tuple, not really needed, but painless as it's only on __init__ time

But this value is a tuple

https://docs.python.org/3/reference/compound_stmts.html#function-definitions

If the form “*identifier” is present, it is initialized to a tuple receiving any excess positional parameters, defaulting to the empty tuple.

Uh.. not sure why I was thinking it was a list, ok, I'll fix, thank you

raiden/utils/runnable.py

raiden/ui/cli.py

hackaugusto · 2018-08-31T11:12:10Z

raiden/utils/runnable.py

+    def stop(self):
+        """ Synchronous stop, gracefully tells run() to exit
+
+        May wait subtasks to finish.


If it's not waiting then it's not synchronous.

Ok, it should be made synchronous

hackaugusto · 2018-08-31T11:12:56Z

raiden/raiden_service.py

+            with gevent.Timeout(self.shutdown_timeout, 'Substasks didn\'t stopped on time'):
+                self.transport.stop()
+                self.alarm.stop()
+                self.transport.get()


what is the point of this call if stop is synchronous?

stop isn't intended to raise run-time exceptions, only stop-time/cleanup ones (as well as start raise startup-time exceptions). This get here ensures run-time exceptions are re-raised, if any.

I don't understand what you mean by runtime/stoptime/cleanup exceptions.

Imagine the life of a Runnable: start -> run -> stop

start is synchorous, and may raise if something during startup didn't go as expected. Think on a port binding when the port is already in use, and it should prevent the task from being actually started (as some pre-start condition failed)

runtime exceptions are raised during "normal" operation, inside run blocking call, either by itself (in case its a loop) or by any subtask (through kill or get)

stop should also be synchronous, and it should fail on cleanup exceptions (imagine a commit on a DB which failed). No need to re-raise run-time exceptions, they should be re-raised by parent at due time with get

The call to stop then get still don't make sense.

If you want to call get because there could have been a "runtime" exception, then the task is dead already and calling stop doesn't make sense.

Runtime exceptions may be raised because of stop. It shouldn't be common, but can happen that stop tells the greenlet to exit gracefully, but then an exception is raised inside it. The interface still holds.

raiden/network/transport/udp/udp_transport.py

hackaugusto · 2018-08-31T13:14:59Z

raiden/utils/runnable.py

+            self.greenlet.name = f'{self.__class__.__name__}|{self.greenlet.name}'
+        self.greenlet.start()
+
+    def run(self, *args, **kwargs):


If you want to stick to the greenlet interface, this should be _run

Yes, I'd do that, just made it without _ to tell its part of the public interface (or at least, the interface children must care), but can make it protected too.

hackaugusto · 2018-08-31T13:16:15Z

raiden/network/transport/udp/udp_transport.py

+
+    def stop(self):
+        if self.stop_event.ready():
+            return  # double call, happens on normal stop, ignore


since start raises, I think calling this on a non started task should raise too.

Yes, this simple return was from a left-over change where stop could be called twice (once for graceful exit, other on run teardown), but since run now only calls stop when raising, this shouldn't be the case anymore and can successfuly raise if called on non-started Runnable.

hackaugusto

Just needs to remove the unused exception, that can go into another PR.

LefterisJP · 2018-08-31T21:35:14Z

This also fixed #1149 ?

andrevmatos · 2018-09-03T07:45:57Z

@LefterisJP yes, most of it. cli.py/app.py still need reorganization, but this removes all the stop timeouts I could find.

hackaugusto · 2018-09-03T10:51:16Z

raiden/network/transport/matrix.py

@@ -514,15 +558,15 @@ def _handle_message(self, room, event) -> bool:
                    "Can't parse message binary data",
                    message_data=data,
                    peer_address=pex(peer_address),
-                    exception=ex,
+                    _exc=ex,


why was this changed? what is the point of this?

exception is used internally by structlog. This threw a lot of warnings in the unit tests @LefterisJP created as part of #2300 . This fixes it by using a non-conflicting parameter name.

This seems the wrong fix IMO, either use log.exception or log.<level>(exc_info=exc)

That would include the whole stacktrace of the exceptions, which isn't required here. Current approach only logs the repr of the exception, which is enough for an entry which can spam the logs if invalid messages are being received.

raiden/network/transport/matrix.py

Its run method will wait on any sub-task (send_with_retry greenlets and GMatrixClient loop), and re-raise if any exception happens.

- RaidenService.start is now a synchronous method, re-raises startup-time exceptions. start_async spawns a greenlet for it, which should be checked for errors - get rid of RaidenService.start_event, uses stop_event where needed - no need to check if running in web3 middleware, children will be gracefully stopped and cleaned.

[ci integration]

andrevmatos added the dev: Please Review label Aug 17, 2018

jomuel mentioned this pull request Aug 17, 2018

Make sure the client crashes on every unhandled exception (new approach) #2201

Closed

4 tasks

hackaugusto reviewed Aug 17, 2018

View reviewed changes

andrevmatos added dev: Work In Progress and removed dev: Please Review labels Aug 22, 2018

andrevmatos mentioned this pull request Aug 23, 2018

Receiving transfers fail with EventUnlockClaimFailed #2213

Closed

hackaugusto suggested changes Aug 23, 2018

View reviewed changes

This was referenced Aug 24, 2018

Don't use a timeout in shutdown sequence #1149

Closed

Fix matrix undefined retries by watching the message queues instead of internal async_results #2268

Merged

andrevmatos mentioned this pull request Aug 28, 2018

Rest API unhandled exceptions should crash the node #2179

Closed

andrevmatos added dev: Please Review and removed dev: Work In Progress labels Aug 30, 2018

hackaugusto suggested changes Aug 31, 2018

View reviewed changes

hackaugusto approved these changes Aug 31, 2018

View reviewed changes

andrevmatos mentioned this pull request Sep 3, 2018

Stop processing the channel queue once the channel is closed #1611

Closed

hackaugusto reviewed Sep 3, 2018

View reviewed changes

raiden/network/transport/matrix.py Show resolved Hide resolved

andrevmatos added 8 commits September 3, 2018 22:07

Add synchronous stop to AlarmTask

cc6500d

Rename transport.stop_and_wait to stop

e2d0372

Add Runnable class

a96e90a

Make MatrixTransport a Runnable

ee7fb5a

Its run method will wait on any sub-task (send_with_retry greenlets and GMatrixClient loop), and re-raise if any exception happens.

Make APIServer a Runnable

e47adfa

Make AlarmTask a Runnable

75af97f

Make UDPTransport a Runnable

461d013

andrevmatos added 8 commits September 3, 2018 22:07

Exit and re-raise if long-lived tasks exited

0cfaddc

Fix xfail on test_crash_on_unhandled_exception

5c02055

[ci integration]

PR review

3416f04

[ci integration]

Remove start/stop_asyncs

00745c5

Fix matrix unit tests

e769125

[ci integration]

Avoid trying to stop api_server if already stopped

9f8b860

[ci integration]

Include node=pex(address) in matrix logs

f3fc6f2

[ci integration]

Avoid double start/stop of raiden

8978fd4

[ci integration]

andrevmatos merged commit d842c5f into raiden-network:master Sep 3, 2018

andrevmatos deleted the exit_longlived_greenlets branch September 3, 2018 21:38

LefterisJP mentioned this pull request Sep 4, 2018

udp: improved logging #2345

Merged

andrevmatos mentioned this pull request Sep 4, 2018

Fix UDP logging #2350

Merged

jomuel mentioned this pull request Sep 7, 2018

Issue 2213 flaky test #2313

Closed

	def stop_task(task, timeout=None):
	try:
	if callable(getattr(task, 'stop', None)):
	task.stop()
	finally:
	if isinstance(task, gevent.Greenlet):
	task.kill()
	task.get(timeout=timeout)
	elif hasattr(task, 'greenlet'):
	task.greenlet.kill()
	task.greenlet.get(timeout=timeout)

Exit longlived greenlets if any of them fails #2208

Exit longlived greenlets if any of them fails #2208

Conversation

andrevmatos commented Aug 17, 2018

Choose a reason for hiding this comment

andrevmatos Aug 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 21, 2018 • edited

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

andrevmatos Aug 23, 2018 • edited

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

LefterisJP commented Aug 25, 2018

andrevmatos commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrevmatos Aug 31, 2018 • edited

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

andrevmatos Aug 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrevmatos Aug 31, 2018 • edited

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

andrevmatos Aug 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto left a comment

Choose a reason for hiding this comment

LefterisJP commented Aug 31, 2018

andrevmatos commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrevmatos Sep 3, 2018 • edited

Choose a reason for hiding this comment

andrevmatos Aug 20, 2018 •

edited

coveralls commented Aug 21, 2018 •

edited

andrevmatos Aug 23, 2018 •

edited

andrevmatos Aug 31, 2018 •

edited

andrevmatos Aug 31, 2018 •

edited

andrevmatos Aug 31, 2018 •

edited

andrevmatos Aug 31, 2018 •

edited

andrevmatos Sep 3, 2018 •

edited