Skip to content

Send heartbeats to upstream connections - #250

Merged
spuun merged 3 commits into
mainfrom
send-upstream-heartbeats-on-interval
Aug 29, 2026
Merged

Send heartbeats to upstream connections#250
spuun merged 3 commits into
mainfrom
send-upstream-heartbeats-on-interval

Conversation

@spuun

@spuun spuun commented Aug 28, 2026

Copy link
Copy Markdown
Member

AMQProxy should send heartbeats to the upstream on interval if no other frame has been sent.

Fixes #210

NOTE:
This is stacked on #252

@spuun
spuun requested a review from a team as a code owner August 28, 2026 08:10
@spuun
spuun force-pushed the send-upstream-heartbeats-on-interval branch from a5192a6 to 9597bbe Compare August 28, 2026 11:13
Comment thread src/amqproxy/upstream.cr Outdated
Comment thread src/amqproxy/upstream.cr Outdated
Comment thread src/amqproxy/client.cr Outdated
@spuun
spuun requested a review from kickster97 August 28, 2026 12:08

@kickster97 kickster97 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread src/amqproxy/upstream.cr
@channel_max : UInt16
@lock = Mutex.new
@remote_address : String
@channel_max : UInt16

@kickster97 kickster97 Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not introduced in this pr, but we declare @channel_max twice

Comment thread src/amqproxy/client.cr
tune_ok, @credentials = negotiate(@socket)
# The largest frame we offer the client in Connection#Tune, the client can
# only negotiate it down, so it's also the limit we enforce when reading
FRAME_MAX = 131_072_u32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declared in 3 places, could maybe be global?

@lukas8219

Copy link
Copy Markdown

Good if we split the docker etc etc changes with the actual changes?

@spuun

spuun commented Aug 29, 2026

Copy link
Copy Markdown
Member Author

Good if we split the docker etc etc changes with the actual changes?

Yes, we'll merge #252 first.

@carlhoerberg
carlhoerberg changed the base branch from main to crystal-1.21 August 29, 2026 10:45
Base automatically changed from crystal-1.21 to main August 29, 2026 11:57
spuun added 3 commits August 29, 2026 12:00
Heartbeats were only sent as a reply to the server's own heartbeats, but RabbitMQ doesn't send any while it's busy delivering to a consumer. An upstream connection that was quiet in the proxy->server direction was then closed by the server with "missed heartbeats from client".
@spuun
spuun force-pushed the send-upstream-heartbeats-on-interval branch from c06cef4 to 2ac53ec Compare August 29, 2026 12:01
@spuun
spuun merged commit b3cfb8e into main Aug 29, 2026
13 checks passed
@spuun
spuun deleted the send-upstream-heartbeats-on-interval branch August 29, 2026 12:30
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.

Getting a lot of Upstream error messages

4 participants