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

internal/protocol: Default timeout for Call #274

Closed

Conversation

cole-miller
Copy link
Contributor

This implements the suggestion to have a default or "fallback" timeout for calls to protocol.Connector.Connect() in #235. The fallback timeout of 30 seconds is used only if the passed context does not already have a deadline configured.

Closes #235

Signed-off-by: Cole Miller cole.miller@canonical.com

@cole-miller cole-miller marked this pull request as ready for review September 18, 2023 16:21
Copy link
Contributor

@MathieuBordere MathieuBordere left a comment

Choose a reason for hiding this comment

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

Can you please just double check if we don't have any logic ourselves that depends on this function retrying forever?

Copy link

@manadart manadart 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 the issue was actually manifest after the connection was established, then subsequently lost.

If you look at Query versus QueryContext for example, they fall through to protocol.Call, where it conditionally sets the conn deadline here.

This is probably where we should also be setting a fall-back.

@cole-miller
Copy link
Contributor Author

@manadart Thanks, I guess I got my wires crossed interpreting that issue thread. Let me confirm that updating the Call logic makes sense and I'll push v2.

If the context for Call doesn't have a deadline, set a default deadline
after 30 seconds. This prevents Call from hanging forever when the
server at the other end is unreachable, if it comes from a database
operation that doesn't take a context (like Exec).

Signed-off-by: Cole Miller <cole.miller@canonical.com>
@cole-miller
Copy link
Contributor Author

Okay, the latest commit sets a default deadline for Call instead of for Connect.

@cole-miller cole-miller changed the title internal/protocol: Default timeout for Connect internal/protocol: Default timeout for Call Sep 21, 2023
@cole-miller cole-miller closed this Jul 5, 2024
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.

Add a default timeout for Connector.Connect
3 participants