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

db.stream(): add timeout #566

Closed

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Aug 8, 2022

Possible fix for #565.

Without fixing gajus/slonik#347 (e.g. with gajus/slonik#360), this will still result in hanging connections sometimes, but is still a significant improvement vs current master.

@alxndrsn alxndrsn marked this pull request as ready for review August 10, 2022 12:47
@alxndrsn
Copy link
Contributor Author

An alternative to setting a static timeout might be to re-set the timeout every time the stream is read from. This would mean that a long-running connection would be allowed to complete, but should still catch idle connections and allow them to be released.

Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

It looks like we specify proxy_read_timeout 2m; in the production nginx config. From a look at the docs for proxy_read_timeout, it looks like that's two minutes between reads. Should we match those settings here?

Or thinking of a different approach entirely, does nginx indicate to Backend when nginx times out the request? If Backend could automatically detect that and clean up the request and its database connections then, that seems ideal. But I imagine that's easier said than done. 😅

@matthew-white
Copy link
Member

@alxndrsn, now that we have #599 and #604, is this PR ready to be closed?

@alxndrsn alxndrsn closed this Sep 19, 2022
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