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

If heartbeats are disabled, do not use a read timeout when reading frame header #551

Merged
merged 2 commits into from
Apr 29, 2018

Conversation

carlhoerberg
Copy link
Contributor

Bug #519 wasn't quite resolved. If heartbeats are disabled (keepalive
used instead), Bunny would use the default read_timeout when waiting for
next frame, that meant that the connection would be closed with an
exception if there was no activity for 30s.

Bug ruby-amqp#519 wasn't quite resolved. If heartbeats are disabled (keepalive
used instead), Bunny would use the default read_timeout when waiting for
next frame, that meant that the connection would be closed with an
exception if there was no activity for 30s.
@carlhoerberg
Copy link
Contributor Author

Yes, this disables heartbeat timeouts as well as they're intertwined. Suggestions welcomed on how to easiest fix that.

@michaelklishin
Copy link
Member

@carlhoerberg I'm not sure I understand the "disables heartbeat timeouts" part. It does interfere with the heartbeat mechanism somewhat, as any socket operation timeout changes do, but they should still work.

I am on the fence about this change. Since it is possible to bump the timeout for the user to a really high value, shouldn't you just do that instead of changing Bunny? FWIW TCP keepalives are not used by the majority of users.

@michaelklishin
Copy link
Member

We can make frame header reads use no timeout only if heartbeats are disabled. That's not very elegant but makes most sense to me conceptually. WDYT @carlhoerberg?

@carlhoerberg
Copy link
Contributor Author

carlhoerberg commented Apr 26, 2018

Disabled read_timeout completely now when heartbeat is disabled too. The behavior of connections with heartbeat enabled is unchanged with this patch.

@michaelklishin
Copy link
Member

michaelklishin commented Apr 26, 2018

Thanks, I will QA this fairly soon. Probably worth a minor version bump for if we end up merging it.

@michaelklishin michaelklishin changed the title No read timeout when waiting for next frame If heartbeats are disabled, do not use a read timeout when reading frame header Apr 28, 2018
@michaelklishin michaelklishin merged commit a92d112 into ruby-amqp:master Apr 29, 2018
@michaelklishin
Copy link
Member

After giving it some thought I think it makes sense to disable TCP socket read timeouts if heartbeats are disabled. The two are interconnected in practice. I left a note about TCP keepalives and a link to RabbitMQ heartbeats guide in the change log.

Thank you, @carlhoerberg.

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

Successfully merging this pull request may close these issues.

2 participants