Repository: mercadona/rele

Issue:
'ThreadPoolExecutor' object has no attribute 'queue'
On Relé 0.9.0 I get the following error when starting a worker:

```
Starting Rele
Traceback (most recent call last):
  File "worker.py", line 12, in <module>
    worker.run_forever()
  File "/usr/local/lib/python3.7/site-packages/rele/worker.py", line 77, in run_forever
    self.start()
  File "/usr/local/lib/python3.7/site-packages/rele/worker.py", line 66, in start
    scheduler=scheduler,
  File "/usr/local/lib/python3.7/site-packages/rele/client.py", line 88, in consume
    subscription_path, callback=callback, scheduler=scheduler
  File "/usr/local/lib/python3.7/site-packages/google/cloud/pubsub_v1/subscriber/client.py", line 228, in subscribe
    manager.open(callback=callback, on_callback_error=future.set_exception)
  File "/usr/local/lib/python3.7/site-packages/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py", line 429, in open
    self._dispatcher = dispatcher.Dispatcher(self, self._scheduler.queue)
AttributeError: 'ThreadPoolExecutor' object has no attribute 'queue'
Sentry is attempting to send 0 pending error messages
Waiting up to 2 seconds
Press Ctrl-C to quit
```

The issue is that we are passing an executor to the [consume API](https://github.com/mercadona/rele/blob/master/rele/worker.py#L66), when in fact it should be an[ instance of a Scheduler](https://googleapis.dev/python/pubsub/1.0.2/subscriber/api/scheduler.html#google.cloud.pubsub_v1.subscriber.scheduler.ThreadScheduler)

Discussion:

Patch:
diff --git a/rele/subscription.py b/rele/subscription.py
index 846618d..73f6b03 100644
--- a/rele/subscription.py
+++ b/rele/subscription.py
@@ -1,7 +1,6 @@
 import json
 import logging
 import time
-from concurrent import futures
 
 from .middleware import run_middleware_hook
 
@@ -64,7 +63,19 @@ class Callback:
         run_middleware_hook("pre_process_message", self._subscription, message)
         start_time = time.time()
 
-        data = json.loads(message.data.decode("utf-8"))
+        try:
+            data = json.loads(message.data.decode("utf-8"))
+        except json.JSONDecodeError as e:
+            message.ack()
+            run_middleware_hook(
+                "post_process_message_failure",
+                self._subscription,
+                e,
+                start_time,
+                message,
+            )
+            run_middleware_hook("post_process_message")
+            return
 
         try:
             res = self._subscription(data, **dict(message.attributes))
diff --git a/rele/worker.py b/rele/worker.py
index 08ea6fa..6b1e901 100644
--- a/rele/worker.py
+++ b/rele/worker.py
@@ -3,6 +3,8 @@ import sys
 import time
 from concurrent import futures
 
+from google.cloud.pubsub_v1.subscriber.scheduler import ThreadScheduler
+
 from .client import Subscriber
 from .middleware import run_middleware_hook
 from .subscription import Callback
@@ -56,9 +58,10 @@ class Worker:
             executor_kwargs = {
                 "thread_name_prefix": "ThreadPoolExecutor-ThreadScheduler"
             }
-            scheduler = futures.ThreadPoolExecutor(
+            executor = futures.ThreadPoolExecutor(
                 max_workers=self.threads_per_subscription, **executor_kwargs
             )
+            scheduler = ThreadScheduler(executor=executor)
             self._futures.append(
                 self._subscriber.consume(
                     subscription_name=subscription.name,


Test patch:
diff --git a/tests/test_subscription.py b/tests/test_subscription.py
index ec33e67..d3e07d5 100644
--- a/tests/test_subscription.py
+++ b/tests/test_subscription.py
@@ -91,14 +91,33 @@ class TestCallback:
             message_id="1",
         )
 
-        return pubsub_v1.subscriber.message.Message(rele_message, "ack-id", MagicMock())
+        message = pubsub_v1.subscriber.message.Message(
+            rele_message, "ack-id", MagicMock()
+        )
+        message.ack = MagicMock(autospec=True)
+        return message
 
     @pytest.fixture
     def message_wrapper_empty(self):
         rele_message = pubsub_v1.types.PubsubMessage(
             data=b"", attributes={"lang": "es"}, message_id="1"
         )
-        return pubsub_v1.subscriber.message.Message(rele_message, "ack-id", MagicMock())
+        message = pubsub_v1.subscriber.message.Message(
+            rele_message, "ack-id", MagicMock()
+        )
+        message.ack = MagicMock(autospec=True)
+        return message
+
+    @pytest.fixture
+    def message_wrapper_invalid_json(self):
+        rele_message = pubsub_v1.types.PubsubMessage(
+            data=b"foobar", attributes={}, message_id="1"
+        )
+        message = pubsub_v1.subscriber.message.Message(
+            rele_message, "ack-id", MagicMock()
+        )
+        message.ack = MagicMock(autospec=True)
+        return message
 
     def test_log_start_processing_when_callback_called(
         self, caplog, message_wrapper, published_at
@@ -129,6 +148,7 @@ class TestCallback:
             res = callback(message_wrapper)
 
         assert res == 123
+        message_wrapper.ack.assert_called_once()
         assert len(caplog.records) == 3
         message_wrapper_log = caplog.records[1]
         assert message_wrapper_log.message == (
@@ -168,6 +188,7 @@ class TestCallback:
         res = callback(message_wrapper)
 
         assert res is None
+        message_wrapper.ack.assert_not_called()
         failed_log = caplog.records[-1]
         assert failed_log.message == (
             "Exception raised while processing "
@@ -187,6 +208,33 @@ class TestCallback:
         }
         assert failed_log.subscription_message == message_wrapper
 
+    def test_log_acks_called_message_when_not_json_serializable(
+        self, caplog, message_wrapper_invalid_json, published_at
+    ):
+        callback = Callback(sub_stub)
+        res = callback(message_wrapper_invalid_json)
+
+        assert res is None
+        message_wrapper_invalid_json.ack.assert_called_once()
+        failed_log = caplog.records[-1]
+        assert failed_log.message == (
+            "Exception raised while processing "
+            "message for rele-some-cool-topic - "
+            "sub_stub: JSONDecodeError"
+        )
+        assert failed_log.metrics == {
+            "name": "subscriptions",
+            "data": {
+                "agent": "rele",
+                "topic": "some-cool-topic",
+                "status": "failed",
+                "subscription": "rele-some-cool-topic",
+                "duration_seconds": pytest.approx(0.5, abs=0.5),
+                "attributes": {},
+            },
+        }
+        assert failed_log.subscription_message == message_wrapper_invalid_json
+
     def test_published_time_as_message_attribute(self, message_wrapper, caplog):
         callback = Callback(sub_published_time_type)
         callback(message_wrapper)
diff --git a/tests/test_worker.py b/tests/test_worker.py
index f8fb506..9d3d41b 100644
--- a/tests/test_worker.py
+++ b/tests/test_worker.py
@@ -3,6 +3,7 @@ from unittest.mock import ANY, patch
 import os
 
 import pytest
+from google.cloud.pubsub_v1.subscriber.scheduler import ThreadScheduler
 
 from rele import Subscriber, Worker, sub
 from rele.middleware import register_middleware
@@ -47,7 +48,8 @@ class TestWorker:
             subscription_name="rele-some-cool-topic", callback=ANY, scheduler=ANY
         )
         scheduler = mock_consume.call_args_list[0][1]["scheduler"]
-        assert isinstance(scheduler, futures.ThreadPoolExecutor)
+        assert isinstance(scheduler, ThreadScheduler)
+        assert isinstance(scheduler._executor, futures.ThreadPoolExecutor)
 
     def test_setup_creates_subscription_when_topic_given(
         self, mock_create_subscription, worker
@@ -71,7 +73,8 @@ class TestWorker:
             subscription_name="rele-some-cool-topic", callback=ANY, scheduler=ANY
         )
         scheduler = mock_consume.call_args_list[0][1]["scheduler"]
-        assert isinstance(scheduler, futures.ThreadPoolExecutor)
+        assert isinstance(scheduler, ThreadScheduler)
+        assert isinstance(scheduler._executor, futures.ThreadPoolExecutor)
         mock_wait_forever.assert_called_once()
 
     @patch.object(Worker, "_wait_forever")