Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FailedRertyCount incorrect usage in SendAsync method of MessageSender (+ no delays being used) #1646

Open
AnakovaK opened this issue Jan 23, 2025 · 2 comments

Comments

@AnakovaK
Copy link

Good time of day to you!

Our team has noticed the unusual behavior of one of the main functions: SendAsync() in MessageSender. Retrying to send a message works in a loop do {...} while() with, supposedly, incorrect loop exit (by variable retry in while):

In method UpdateMessageForRetry, main question is that line:var retryCount = Math.Min(_options.Value.FailedRetryCount, 3);. What is the point of turning default value of 50 in FailedRetryCount (or any other which is bigger than 3) into only 3 retries by Math.Min function? All three of retries fire instantly as well through the retry loop: the delay is not used anywhere in said file (e.g. FailedRetryInterval in options).

Supposedly, the function was meant to be Math.Max, making the minimum amount of retries 3 (if someone wanted to set FailedRetryCount to 3 or less). But that is not perfect as well, as it does not cover the case of someone wanting to turn off retries completely.

With the above change, the if (retries == _options.Value.FailedRetryCount) should be turned into if (retries >= _options.Value.FailedRetryCount), due to unguaranteed consistency of amount of retries (e.g., several inside workers tried to send the same message and got the retries amount over the limit). (Thanks to @CAPCHIK for poiting out the corrections and their issues).

I'm willing to open MR with these changes if in fact the proposed changes are supposed to change behaviour for the better.

@PoteRii
Copy link
Contributor

PoteRii commented Jan 26, 2025

@yang-xiaodong
Copy link
Member

yang-xiaodong commented Feb 4, 2025

Retries are designed to be retried immediately 3 times without using an interval.

if (retries == _options.Value.FailedRetryCount)

The FailedThresholdCallback configuration item is designed to be triggered only once when FailedRetryCount is reached, rather than continuing to be triggered when it is greater.

several inside workers tried to send the same message and got the retries amount over the limit

Do you mean there are multiple instances of concurrent retries?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants