✨ rabbit_client.subscribe no longer retries forever if message_handler always raises unexpected error ⚠️

[GitHK]

What do these changes do?

Currently if the message_handler raises an exception, the message is queued again to be retried.
While this behaviour is useful, in some situations is results being decremental.

Issues with the current design:

• the message is retired immediately
• if message_handler always raises the message will be retried forever and can overwhelm the system
• does not allow for handlers to fail and stop after X failed attempts

---

Changes

Added dead letter exchange box rabbit_client.subscribe internal queue, which helps to deal with errors.

• ✨ added unexpected_error_retry_delay_s which delays the message by a certain amount before queuing it again in the main queue
• ✨ added unexpected_error_max_attempts which allows to limit how many times the message_handler is allowed to raise an error before giving up
• ✅ if message_handler returns False forever, the message will always be queued immediately to the main queue and will be retried as soon as possible (did not change previous behaviour)

Related issue/s

How to test

Dev Checklist

• No ENV changes or I properly updated ENV (read the instruction)

DevOps Checklist ⚠️

• ⚠️ NOTE: queue signatures in rabbitmq have changed. services which rely on rabbit_cliebt.subscribe will no longer be able to start. We need to remove the queues (volumes) from rabbitmq in oder for the services to start

Why not have an automatic way to fix this?

• read reasons here ✨ added notes with actions to take if rabbitmq queues are no longer created #5295

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7821b90) 77.9% compared to head (489b408) 87.1%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #5282      +/-   ##
=========================================
+ Coverage    77.9%   87.1%    +9.2%     
=========================================
  Files        1029    1316     +287     
  Lines       40683   53839   +13156     
  Branches      765    1173     +408     
=========================================
+ Hits        31700   46932   +15232     
+ Misses       8833    6657    -2176     
- Partials      150     250     +100     

Flag	Coverage Δ
integrationtests	63.8% <ø> (?)
unittests	85.1% <97.2%> (+7.2%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ls-library/src/models_library/rabbitmq_messages.py	89.7% <ø> (ø)
...y/src/servicelib/aiohttp/monitor_slow_callbacks.py	100.0% <100.0%> (ø)
...service-library/src/servicelib/rabbitmq/_client.py	100.0% <100.0%> (ø)
.../service-library/src/servicelib/rabbitmq/_utils.py	97.0% <75.0%> (ø)

... and 455 files with indirect coverage changes

[sanderegg]

I have some doubts still on the necessity of this change.

1. so if a message is never delivered, we need to look for the exception in graylog right? what about the dead end letter box pattern thingy? did you check that?
2. what is the use-case where the handler would constantly fail?
3. what is a good number for the failures? and you are saying there is no delay, how will this work?

[GitHK]

I have some doubts still on the necessity of this change.

1. so if a message is never delivered, we need to look for the exception in graylog right? what about the dead end letter box pattern thingy? did you check that?

I did not manage to get it working right, but I think I did not do it right. Might still end up requiring it, since it looks like to the goto way to implement a delayed message delivery.

EDIT:
I managed to have the dead letter box pattern working. And with this one I can implement the delay approach. Will try to bring something in using this one now.

2. what is the use-case where the handler would constantly fail?

My use case is the following: I emit messages for some small task, that I know can fail sometimes, due to the state of the system. I want it to actually retry 0 times and give up.

3. what is a good number for the failures? and you are saying there is no delay, how will this work?

Regarding the no delay: I've seen 1000+ fails in one second on my machine. I think it's a bad idea and could lead to some strange situations or performance going down.

Regarding the retry policy, I would put something similar to what tenacity does with the exponential backoff and some max duration.
EDIT: exponential backoff is a bit harder to achieve, for now we have a fixed delay and a maximum number of retries, after which an error is logged when the handler gives up on the message.

---

@pcrespov @sanderegg I have also updated the description of the PR and this is the new design.

After the new change. A message will always be retried a set number of times and it is given up if the error keeps raising unexpected errors.

NOTE: if the handler returns (False) the message will be send to the original queue, not the one handling the errors. So previous behaviour is unaltered.

A warning message is printed out when the handler fails and the message will be queue once more after a fixed delay.

WARNING  servicelib.rabbitmq._client:_client.py:59 Retry [0/0] for handler '<function test__subscribe_to_failing_message_handler.<locals>._faulty_message_handler at 0x7fdb4b0a9000>', which raised an unexpected error caused by message_id='5b757bf24e734dcdb74940c970646f11'

An error message will be printed out if the message has reached its maximum amount of queued times and the it's given up on trying to handle it.

ERROR    servicelib.rabbitmq._client:_client.py:74 Handler '<function test__subscribe_to_failing_message_handler.<locals>._faulty_message_handler at 0x7fdb4b0a9000>' is giving up on message 'IncomingMessage:{'app_id': None,
 'body_size': 181,
 'cluster_id': '',
 'consumer_tag': 'pytest_consumer_neagu-wkst_3113883_pytest_fake_exchange_qaUauvFYAmzYNIRRwotb',
 'content_encoding': None,
 'content_type': None,
 'correlation_id': None,
 'delivery_mode': <DeliveryMode.NOT_PERSISTENT: 1>,
 'delivery_tag': 1,
 'exchange': 'pytest_fake_exchange_qaUauvFYAmzYNIRRwotb',
 'expiration': None,
 'headers': {},
 'message_id': '5b757bf24e734dcdb74940c970646f11',
 'priority': 0,
 'redelivered': False,
 'reply_to': None,
 'routing_key': '',
 'timestamp': None,
 'type': 'None',
 'user_id': None}' with body 'b'American whole magazine truth stop whose. On traditional measure example sense peace. Would mouth relate own chair.\nTogether range line beyond. First policy daughter need kind miss.''
Traceback (most recent call last):
  File "/home/silenthk/work/pr-osparc-retry-on-error-queue/packages/service-library/src/servicelib/rabbitmq/_client.py", line 54, in _on_message
    if not await message_handler(message.body):
  File "/home/silenthk/work/pr-osparc-retry-on-error-queue/packages/service-library/tests/rabbitmq/test_rabbitmq.py", line 515, in _faulty_message_handler
    raise RuntimeError(msg)
RuntimeError: Always fail. Received message b'American whole magazine truth stop whose. On traditional measure example sense peace. Would mouth relate own chair.\nTogether range line beyond. First policy daughter need kind miss.'

[sanderegg]

thanks

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud