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

Add slow-speed timeout when downloading mod #611

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Dec 2, 2023

Adds a timeout if connection is too slow.


Previous PR text:

 

While currently in a business trip, not having access to a super duper fiber Internet connection, I tried downloading some mods, which failed due to the poor Internet connection: mod request would timeout before mod is downloaded.

[2023-12-02] [11:37:16] [NORTHSTAR] [info] Mods list successfully fetched.
[2023-12-02] [11:37:16] [NORTHSTAR] [info] Loading mods configuration...
[2023-12-02] [11:37:16] [NORTHSTAR] [info] ==> Loaded configuration for mod "Odd.s2space"
[2023-12-02] [11:37:16] [NORTHSTAR] [info] Done loading verified mods list.
[2023-12-02] [11:37:32] [NORTHSTAR] [info] Fetching mod archive from https://gcdn.thunderstore.io/live/repository/packages/odds-s2space-0.0.5.zip
[2023-12-02] [11:37:32] [NORTHSTAR] [info] Downloading archive to C:/users/steamuser/Temp/odds-s2space-0.0.5.zip
[2023-12-02] [11:38:02] [NORTHSTAR] [error] Fetching mod archive failed: Timeout was reached
[2023-12-02] [11:38:02] [NORTHSTAR] [error] Something went wrong while fetching archive, aborting.

Looking in the code, I realized I put the same timeout for the mod fetching request than for the mods list fetching request, which is 30 seconds.

I thought about setting the timeout with a higher value (a few minutes or so), but then I remembered when I was a little boy, I would leave my computer running overnight to download games because my Internet connection was so shitty :>

In this PR, I ended up completely removing the timeout attribute, meaning mod fetching requests will never time out (according to curl documentation).

[2023-12-02] [12:50:45] [NORTHSTAR] [info] Mods list successfully fetched.
[2023-12-02] [12:50:45] [NORTHSTAR] [info] Loading mods configuration...
[2023-12-02] [12:50:45] [NORTHSTAR] [info] ==> Loaded configuration for mod "Odd.s2space"
[2023-12-02] [12:50:45] [NORTHSTAR] [info] Done loading verified mods list.
[2023-12-02] [12:50:56] [NORTHSTAR] [info] Fetching mod archive from https://gcdn.thunderstore.io/live/repository/packages/odds-s2space-0.0.5.zip
[2023-12-02] [12:50:56] [NORTHSTAR] [info] Downloading archive to C:/users/steamuser/Temp/odds-s2space-0.0.5.zip
[2023-12-02] [12:51:48] [NORTHSTAR] [info] Mod archive successfully fetched.

/storytelling off

@GeckoEidechse
Copy link
Member

Wait, the timeout kicks even though progress is made? As long as there's still an error when connection is dropped entirely (no progress made) this is fine to merge by me ^^

@Alystrasz
Copy link
Contributor Author

Alystrasz commented Dec 2, 2023

This is not straightforward to implement, but I suppose I could implement such a behavior using CURLOPT_LOW_SPEED_TIME.
Would you like me to do that in this PR?

EDIT: done in fb8ebf6

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Changes look good, definitely preferable to a hard timeout

@ASpoonPlaysGames ASpoonPlaysGames added the needs testing Changes from the PR still need to be tested label Dec 4, 2023
@GeckoEidechse GeckoEidechse added the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Dec 13, 2023
@GeckoEidechse
Copy link
Member

For testing I guess the easiest way would be to yank the ethernet cable half way through download?

Not sure if we even need testing for it though as the feature as a whole is still considered unreleased as it's gated behind convar.

@Alystrasz
Copy link
Contributor Author

For testing I guess the easiest way would be to yank the ethernet cable half way through download?

I did the testing by cutting the Wi-Fi signal, but it's the same idea, yeah.

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Dec 27, 2023
@GeckoEidechse GeckoEidechse removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jan 20, 2024
@GeckoEidechse
Copy link
Member

Given that you already tested it, I'm skip testing on my end and just merge directly instead ^^

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jan 20, 2024

wait, where did the arg removal go? Hol' up.

@GeckoEidechse
Copy link
Member

Oh wait, so the change from c656859 was already merged in #595. Guess I'm renaming this PR here then so it's a bit better reflecting of the actual change, lol

@GeckoEidechse GeckoEidechse changed the title fix: Remove mod fetching request timeout Add slow-speed timeout when downloading mod Jan 20, 2024
@GeckoEidechse
Copy link
Member

Okay, now it should be ready to merge, lol

@GeckoEidechse GeckoEidechse merged commit bf7b5e0 into R2Northstar:main Jan 20, 2024
3 checks passed
@Alystrasz Alystrasz deleted the fix/mad-timeout branch January 21, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge needs testing Changes from the PR still need to be tested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants