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

Allow persistent=false to be set externally #27

Merged
merged 2 commits into from
Oct 29, 2023

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Oct 27, 2023

This PR allows connection.persistent = false to be called externally.

Use cases:

  1. Server connections - As part of a more graceful shutdown, I want in-flight connections to be allowed to finish the current request/response, but then terminate and not be reused. In addition to closing the connection, flagging connections in this way allows them to return a Connection: closed header so the client is properly notified that this connection isn't to be reused for further requests.

  2. Client connections - When certain conditions are met, I want to flag client connections as not eligible to be returned to the Pool for reuse. This could be because the connection configuration has changed, a maximum request count has been reached, etc.

Alternatives

I'm not sure setting connection.persistent = true is ever warranted. As such, I considered adding a #not_persistent! method instead that only sets to false. I chose to go with the simpler solution of an accessor, but going with something more opinionated is an option if preferred.

Types of Changes

  • New feature.

Contribution

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

I think we should expand on the documentation a little and also add a warning about setting persistent to true when the other end is not expecting it.

@zarqman
Copy link
Contributor Author

zarqman commented Oct 28, 2023

Updated!

@ioquatix ioquatix merged commit 6b92ded into socketry:main Oct 29, 2023
16 of 17 checks passed
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