search

vert-x3 / vertx-amqp-client

An AMQP client for Vert.x
Apache License 2.0
17 stars 18 forks source link

Fix not to lose messages while handling message and executing a scheduled message delivery async event #39

Closed ferhatsahinkaya closed 4 years ago

ferhatsahinkaya commented 4 years ago

Fix not to lose messages while handling message and executing a scheduled message delivery async event

Do not decrement demand in scheduleBufferedMessageDelivery method when there is no message in the buffer Deliver message to the handler immediately in handleMessage method when there is demand and buffer is not empty

Motivation:

This commit fixes #36, fixes #37 and fixes #38. We faced with these issues in our production environment. Occasionally, our amqp consumers stop consuming messages due to issue #38 so that we need to restart our pods having the issue in order to start consuming messages again. When demand is decremented incorrectly due to issue #38, there is no chance to recover as already placed demand is lost. After understanding how AmqpReceiver works and finding a solution to our problem, I have realised there is a chance to lose messages due to issues #36 and #37.

A new ReceiverTest is added as part of the commit. As all issues regarding the fix are caused due to race conditions and existing tests are integration tests rather than unit tests, in order to keep the consistency in the code base, I’ve added a new similar integration test to ReceiverTest class.

The aim of the new test is to catch the cases below: receiving message from ProtonReceiver (handleMessage method) when there are already messages in the buffer and demand is greater than zero having buffer empty during execution of the async event scheduled in scheduleMessageDelivery method

In order to catch these cases, I sent two 1K batches to the broker and in between batches, I increased the demand to catch the above cases. However, technically, due to the existing testing style (integration testing receier with the broker) and limited accessibility to the internals of the AmqpReceiver, added test only increases the chances of the failure but doesn’t guarantee hitting the cases in the issues as there is no easy way to control the thread scheduling to process asynch events for dispatching messages to handlers or fetching messages from broker to the buffer. Please feel free to reach out to me in case of any concern.

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

vietj commented 4 years ago

can you review this one @cescoffier ?

cescoffier commented 4 years ago

This would need to be backported to 3.9.2.

ferhatsahinkaya commented 4 years ago

@cescoffier thanks for the review. Do I need to do anything else to get the fix merged with 3.9 and master branches? Unfortunately, couldn't see any clear process to follow.

cescoffier commented 4 years ago

@ferhatsahinkaya I will do it, that's a reminder for me.

cescoffier commented 4 years ago

@vietj - Backport done in the 3.9 branch.

vietj commented 4 years ago

thanks

On Tue, Jun 16, 2020 at 8:02 AM Clement Escoffier notifications@github.com wrote:

@vietj https://github.com/vietj - Backport done in the 3.9 branch.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-amqp-client/pull/39#issuecomment-644549865, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCUSC4MVA3NA4NGBYCDRW4DHXANCNFSM4N3W7SUA .