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

feat: add client_idle_timeout option #165

Merged
merged 6 commits into from
Sep 18, 2023
Merged

feat: add client_idle_timeout option #165

merged 6 commits into from
Sep 18, 2023

Conversation

abc3
Copy link
Member

@abc3 abc3 commented Sep 14, 2023

This PR adds a configurable option to prevent zombie issues and enables keepalive in incoming connections.


  • If a client stays connected but doesn't send any messages over the connection for N milliseconds, then disconnect the client
  • Disabled by default
  • Put on tenant record
  • Name it client_idle_timeout or idle_timeout_downstream??
  • Set keepalive in ranch to true

@abc3 abc3 linked an issue Sep 14, 2023 that may be closed by this pull request
4 tasks
@chasers
Copy link
Contributor

chasers commented Sep 14, 2023

Let’s call this the same thing as pgbouncer client_idle_timeout

@abc3
Copy link
Member Author

abc3 commented Sep 14, 2023

the terms downstream/upstream offer more specific and clear definitions and we are already using them in the project

@chasers
Copy link
Contributor

chasers commented Sep 14, 2023

Yeah but people will be looking for equivalent configs.

@abc3 abc3 requested review from a team and chasers September 14, 2023 17:14
@josevalim
Copy link
Contributor

👍 on client_idle_timeout.

The current implementation is great as is, because it seems you only want to enable the timeout in certain scenarios. Although I believe the current implementation would not close the connection if it asks for a query and then forgets (or is too slow) to read it for some reason, right?

In any case, if you wanted to enable the timeout on most handle_event returns, then I am not the biggest fan of gen_statem/gen_server timeouts because if you forget to return timeout in a single callback, then it may no longer trigger as expected.

@abc3
Copy link
Member Author

abc3 commented Sep 15, 2023

Although I believe the current implementation would not close the connection if it asks for a query and then forgets (or is > too slow) to read it for some reason, right?

Yeah, that is not covered

In any case, if you wanted to enable the timeout on most handle_event returns, then I am not the biggest fan of gen_statem/gen_server timeouts because if you forget to return timeout in a single callback, then it may no longer trigger as expected.

@josevalim, what would you recommend using for timeouts? Should I use Process.send_after? I'll need to add a bunch of other timeouts soon

@josevalim
Copy link
Contributor

It really depends on the nature of timeouts. Sometimes send_after. Sometimes you can store timestamps on the state and compare when messages arrive, etc. Once you have more details, let us know, and we will gladly discuss :)

@abc3 abc3 changed the title feat: add idle_timeout_downstream option feat: add client_idle_timeout option Sep 18, 2023
@abc3 abc3 merged commit d4c6d65 into main Sep 18, 2023
2 checks passed
@abc3 abc3 deleted the feat/idle_timeout branch September 18, 2023 10:54
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.

Disconnect stale client connections
4 participants