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

Taking out my arbitrary timeouts #864

Merged
merged 3 commits into from
Dec 3, 2020
Merged

Conversation

tanema
Copy link
Contributor

@tanema tanema commented Dec 3, 2020

fixes #857

In previous work, I added in a bunch of arbitrary timeouts, this was because I wanted requests to fail faster if there was an issue. This however was misguided because the actual solution was cancelling the requests. The timeouts were also copied over from our webhook delivery service that I worked on and themekit and that service have vastly different request requirements. Webhook delivery requires fast completion on delivery, however themekit is used to uploading very large files that need more time to complete, so this was done in error.

I have just removed all of these timeouts as the defaults are either pretty large or no timeout is set at all. There is still a general overall request timeout so that does not mean requests will get stuck.

This fix has been confirmed in the issue linked

Warn Checklist

  • This changes the interface and requires a Major/Minor version change.
  • I have 🎩'd these changes by using the commands I changed by hand.
  • I have added a dependancy to the project.
  • I have considered any potential impact on node-themekit

@tanema tanema merged commit 4e9ac30 into master Dec 3, 2020
@tanema tanema deleted the remove_arbitrary_timeouts branch December 3, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant