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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heartbeat vs. hearbeat timeout confussion #127

Closed
vanaoff opened this issue Nov 1, 2023 · 8 comments
Closed

Heartbeat vs. hearbeat timeout confussion #127

vanaoff opened this issue Nov 1, 2023 · 8 comments

Comments

@vanaoff
Copy link

vanaoff commented Nov 1, 2023

Hello there 馃憢

I'm a bit confused of heartbeat parameter. It seems it's value is being used on two places - amqpstorm.channel0.Channel0._send_tune_ok and amqpstorm.heartbeat.Heartbeat._start_new_timer. As far as I understand, the first usage negotiates heartbeat_timeout with the server and the second usage sets the interval to send heartbeat requests.

I would expect the interval should be shorter than timeout, eg. heartbeat_timeout / 2 as described eg. in RabbitMQ documentation here but the amqpstorm uses the same value. Please, what I'm missing here?

Thanks for the answer and all the work on this library 馃檹

Ivan

@eandersson
Copy link
Owner

I don't remember the exact details, but you are essentially setting the interval meaning how often to send heartbeats, not the actual timeout and the code will check if you missed the heartbeat X number of times.

e.g. if you look at the client side code for amqpstorm it checks if you missed two or more heartbeats (meaning interval * 2) before it raises an issue.
https://github.com/eandersson/amqpstorm/blob/2.x/amqpstorm/heartbeat.py#L88

Also, worth nothing that on the client (and maybe server) we count any type of incoming data as a heartbeat. It does not necessarily need to be an actual heartbeat frame.

@vanaoff
Copy link
Author

vanaoff commented Nov 6, 2023

Thank you for the answer 馃憤 I understand the client part. What confuses me is server side of heartbeat. It's not very clear to me what server does in case it misses heartbeat.

What we experience is that some "long-time" (=longer than heartbeat) processing messages get requeued before acknowledgement after time period suspiciously similar to heartbeat.

So here's my suspicion. As described here, there could be mis-alignment between heartbeat_interval and heartbeat_timeout, where heartbeat_interval should be equal to heartbeat_timeout / 2. According to RabbitMQ documentation, it seems RabbitMQ (and official libraries) server exposes heartbeat_timeout, which I think is configured via negotiation in amqpstorm.channel0.Channel0._send_tune_ok... However, the ampqstorm sets the same value to what I think is more like heartbeat_interval. So, when the value is set to eg. 60s, ampqstorm sends hearbeat every 60s and it's correctly handling one miss which makes the actual heartbeat_timeout 120s. On the other hand server closes the connection after 60s of inactivity and requeues the message.

I can try to create reproducible example to prove/disprove this hypothesis...

@eandersson
Copy link
Owner

I see what you mean. I think the terminology is confusing here. I prepared a PR. If you are able can you review it and try it out?

@vanaoff
Copy link
Author

vanaoff commented Nov 10, 2023

Agree, it's confusing. Thanks 馃檹 I'll get to test probably sometime next week. Btw, aren't you worried about backward compatibility?

@eandersson
Copy link
Owner

eandersson commented Nov 10, 2023

Agree, it's confusing. Thanks 馃檹 I'll get to test probably sometime next week. Btw, aren't you worried about backward compatibility?

So there shouldn't be any changes that aren't backwards compatible here since we are keeping the variable names the same. The only risk is that there will be a very minor increase in CPU and bandwidth usage since the background thread is run twice as often now with the same heartbeat value, but we are looking at a very minor increase, since the server already enforced the same current heartbeat value and behaviour.

eandersson added a commit that referenced this issue Nov 15, 2023
Update heartbeat interval implementation [#127]
@vanaoff
Copy link
Author

vanaoff commented Nov 21, 2023

Hello Eric, please, are you planning to release this fix anytime soon?

@eandersson
Copy link
Owner

Hello Eric, please, are you planning to release this fix anytime soon?

Thanks. It should be live now.

@vanaoff
Copy link
Author

vanaoff commented Nov 21, 2023

Thanks once again 馃憤

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

2 participants