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

Change default timeout to none #145

Merged
merged 3 commits into from
Dec 21, 2016
Merged

Change default timeout to none #145

merged 3 commits into from
Dec 21, 2016

Conversation

yusefnapora
Copy link
Contributor

Per conversation with @vyzo in slack, the global request timeouts were causing a lot of problems for operations that take awhile to finish (e.g. publish, etc).

We talked about removing the global timeout and making certain operations opt-in, for example fetching the remote id for a peer. I started to do that, but I couldn't come up with a compelling reason to keep the timeouts at all, versus just passing on the error from concat when its timeouts are exceeded.

With timeouts on the mcclient -> mcnode http connection, you get a confusing error message, and you may end up timing out before mcnode would have given you a more useful message. I'm now thinking that if we're going to have a timeout parameter, it should be sent as part of the HTTP request (e.g in a query param), so that concat can alter its timeout.

Is there something I'm missing here, where timeouts on the API requests buy us something useful?

@vyzo
Copy link
Contributor

vyzo commented Dec 21, 2016

I think we should keep the option, and just default it to 0 (no timeout) instead of removing it altogether.

It can be useful in dealing with slow peers, for instance when doing a listPeers -i where we know if it takes longer than 5s it's almost certainly going to be an error in the client side and we prefer to just return promptly.

Also, we will want to use it to effect concat timeouts when we have this functionality.

@yusefnapora
Copy link
Contributor Author

That's a very sensible plan; I like it 😄

@yusefnapora yusefnapora changed the title Remove API timeouts Change default timeout to none Dec 21, 2016
@yusefnapora yusefnapora merged commit 6f54ed9 into master Dec 21, 2016
@yusefnapora yusefnapora deleted the yn-timeouts branch December 21, 2016 15:34
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