Skip to content

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jun 30, 2025

This PR adds some cleanup logic for rabbitmq websocket connections that have been inactive for some configurable amount of time.

https://wearezeta.atlassian.net/browse/WPB-17895

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jun 30, 2025
@pcapriotti pcapriotti marked this pull request as ready for review June 30, 2025 08:23
@pcapriotti pcapriotti requested review from a team as code owners June 30, 2025 08:23
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

Have you seen the function withPingPong from websockets? Perhaps we don't have to implement this.

Comment on lines 45 to 46
activityTimeout = 30000000, -- TODO
pongTimeout = 30000000 -- TODO
Copy link
Member

Choose a reason for hiding this comment

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

Pending TODOs. Perhaps these value should come from configs and maybe we can write a test then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something went wrong when I last rebased, it seems some commits got lost.

@pcapriotti
Copy link
Contributor Author

Have you seen the function withPingPong from websockets? Perhaps we don't have to implement this.

I've seen it, but I can't see how to use it to implement our desired behaviour (i.e. only submit pings after inactivity).

@akshaymankar
Copy link
Member

I've seen it, but I can't see how to use it to implement our desired behaviour (i.e. only submit pings after inactivity).

Hmm, perhaps its ok to just send pings all the time? The point of all this is to kill connections which have inactive clients.

@pcapriotti
Copy link
Contributor Author

Sorry about the broken PR. Should be ok to review now.

Co-authored-by: Akshay Mankar <[email protected]>
@pcapriotti pcapriotti merged commit a829a8c into develop Jul 11, 2025
8 checks passed
@pcapriotti pcapriotti deleted the ping-timeout branch July 11, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants