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 configurable timeout on Connection #1065

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

silverbucket
Copy link
Contributor

@silverbucket silverbucket commented Feb 16, 2025

The framework for passing in options was already there, I just added the possibility for a user-defined timeout.

@silverbucket
Copy link
Contributor Author

What do you think @sonnyp can we get this merged and a release made?

@sonnyp
Copy link
Member

sonnyp commented Feb 19, 2025

Thanks for the contribution. Can you help me understand why you need this?

@silverbucket
Copy link
Contributor Author

silverbucket commented Feb 19, 2025

@sonnyp well, it seems like the groundwork was already laid for this, just a matter of giving the user the ability to set longer timeouts for connect. Imagine bad latency or unresponsive/slow server response. The default of 2s might not be enough time.

@silverbucket
Copy link
Contributor Author

For more context, in sockethub, we are adding the ability to allow for setting a custom a connect timeout if needed, as everyones network conditions vary. Done for IRC, and HTTP Fetches, just need the ability for XMPP. sockethub/sockethub#894

@sonnyp
Copy link
Member

sonnyp commented Feb 19, 2025

Thanks for the context.

timeout isn't an exposed option at the moment
I don't think this works the way you think it does.

A bit more work/testing is needed

@silverbucket
Copy link
Contributor Author

@sonnyp you're right, it could be simplified. I was trying to go along with the way the options were handled, but now see that while it worked, it was unnecessary.
I've just pushed changes, and have added tests.

@silverbucket
Copy link
Contributor Author

ping @sonnyp this PR is ready for another look, I've addressed your concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants