[service-bus] Fix message loss issues with peekLock and receiveAndDelete #15989
Conversation
…essage from rhea. The message loss issue is related to how we trigger 'drain' using 'addCredit(1)'. Our 'receiver.drain; receiver.addCredit(1)' pattern actually does add a credit, which shows up in the flow frame that gets sent for our drain. This has led to occasionally receiving more messages than we intended. The second part of this was that we were masking this error because we had code that specifically threw out messages if more arrived than were requested. If the message was being auto-renewed it's possible for the message to appear to be missing, and if we were in receiveAndDelete the message is effectively lost at that point. That code is now removed (we defer to just allowing the extrra message, should a bug arise that causes that) and we log an error indicating it did happen. The rhea error message appeared to be triggered by our accidentallly allowing multiple overlapping 'drain's to occur (finalAction did not check to see if we were _already_ draining and would allow it to happen multiple times). Removing the concurrent drains fixed this issue but I didn't fully investigate why.
ghost
added
the
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
I see you added a stress test scenario. Were you ever able to find a way to reliably reproduce the issue in a controlled environment? I didn't see any non-stress tests added.
If you emitted an additional message (amqp onMessage) after you call addCredit(1) with drain set to true, would that trigger it as well? Or does it have to go through rhea?
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… the event listener was registered, rather than when the message handler was called. Fixing this revealed a couple of spots that were incorrectly using the wrong timeout. Also, storing off the finalAction in an internal member variable to maek some testing easier.
…ode that was trying to be a bad eventemitter. Replaced it with an actual eventemitter instead. This had a nice byproduct of removing more code. I also did some renames of variables because `receiver` is no longer a descriptive word (BatchingReceiver, BatchingReceiverLite, ServiceBusReceiver, etc..).
Prior to the fixes I could definitely reproduce this. The stress test went the extra mile of hooking into the internal onMessage() call (before any interpretation by our code) which was flagging the error much earlier.
For the real world failure you'd need to go through rhea, and even then it's possible that you don't get an extra message. Even when I was reproducing it it was sproadic. The only guarantee was that, somewhere in the 200 receives (of 5 messages apiece) to AUS I would see it at least a couple of times. The reality is that this could have been happening for a long while and it would be hard to notice - the code that was previously throwing away 'excess' messages was basically masking the problem. For a unit test (which is what I've added in the push I did just now) I basically just simulate calling finalAction() and checking the before/after credit values (and ensuring I don't try to do concurrent drain requests). That test is pretty simple, but combined with the stress test, gives me good confidence we've hit the key area. |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
(previous live test run passed, just want to be sure so I'll be launching it a few times) |
richardpark-msft
requested review from
deyaaeldeen,
mikeharder,
praveenkuttappan and
witemple-msft
as code owners
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ange so I'm just rolling that back. We'll release core-amqp at some point in the future, no need to do it now.
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixing an issue where we could lose messages or provoke an alarming message from rhea (
Received transfer when credit was 0)The message loss issue is related to how we trigger 'drain' using 'addCredit(1)'. Our 'receiver.drain; receiver.addCredit(1)' pattern actually does add a credit, which shows up in the flow frame that gets sent for our drain. This has led to occasionally receiving more messages than we intended.
The second part of this was that we were masking this error because we had code that specifically threw out messages if more arrived than were requested. If the message was being auto-renewed it's possible for the message to appear to be missing, and if we were in receiveAndDelete the message is effectively lost at that point. That code is now removed (we defer to just allowing the extrra message, should a bug arise that causes that) and we log an error indicating it did happen.
The rhea error message appeared to be triggered by our accidentally allowing multiple overlapping 'drain's to occur (finalAction did not check to see if we were already draining and would allow it to happen multiple times). Removing the concurrent drains fixed this issue but I didn't fully investigate why.
Fixes #15606, #15115