search

cybozu-go / aptutil

Go utilities for Debian APT repositories
MIT License
124 stars 29 forks source link

[mirror] Set download timeout #43

Closed takumin closed 1 month ago

takumin commented 5 years ago

follow cacher

https://github.com/cybozu-go/aptutil/blob/master/cacher/cacher.go#L27 https://github.com/cybozu-go/aptutil/blob/master/cacher/cacher.go#L296

nishitaniyuki commented 5 years ago

Thanks for working on this. Still WIP??

takumin commented 5 years ago

Thank you for the review!

I think it would be better to change it for the following reasons.

  1. It is more elegant to set the properties of http.Client and http.Transport and net.Dialer
  2. context.Context is propagated and not retried

Please merge if there is no problem with the current code.

Reference:

nishitaniyuki commented 5 years ago

In case HTTP request is not completed in 30 minutes, no need to retry, I think. How about setting timeout on download, not on HTTP request?

https://github.com/cybozu-go/aptutil/pull/43/files#diff-65f38e03d73008242435b67f427bb10cR330 And putting these lines(L330-331) on the beginning of download, it's easier to understand that timeout is set on download.

takumin commented 5 years ago

In case HTTP request is not completed in 30 minutes, no need to retry, I think.

I believe that you should set timeouts for individual connections. Then the download will be retried properly and you will be able to recover from a temporary malfunction (network, server etc). Please check the code example.

https://github.com/takumin/aptutil/commit/dce2fee3828877138eba15793dfd436461e8dc54#diff-65f38e03d73008242435b67f427bb10cR31

How about setting timeout on download, not on HTTP request?

I'm sorry. I was not able to accurately capture your intentions. Are the above code examples contrary to your intentions?

And putting these lines(L330-331) on the beginning of download, it's easier to understand that timeout is set on download.

The above code example is written just below RETRY. In this case, while retrying properly, it is canceled by the context propagation only when it is in an unintended deadlock etc. What do you think?

nishitaniyuki commented 5 years ago

I believe that you should set timeouts for individual connections. Then the download will be retried properly and you will be able to recover from a temporary malfunction (network, server etc). Please check the code example.

takumin@dce2fee#diff-65f38e03d73008242435b67f427bb10cR31

Ah, I got it. Can you update PR??

takumin commented 5 years ago

I am sorry that the update of PR is late.

When I tested launchpad.net mirroring with the following settings, I confirmed that the 30 minute timeout was insufficient.

go-apt-mirror.toml (gist)

The reason is that when the launchpad.net server is heavily loaded, timeouts occur frequently depending on the setting of MaxIdleConnsPerHost.

For example, when go-apt-mirror is regularly executed with systemd.timer, it will always fail if the set start time is overlapped with the congested time.

Although this change is being retried several times, it has been confirmed that the synchronization has been completed properly over several hours.

Please merge if there is no problem with this change.

thank you for reading.

nishitaniyuki commented 5 years ago

Sorry for the late reply. I'll take the time to review your PR in this week. Please wait a little longer 🙏

nishitaniyuki commented 5 years ago

@takumin

The reason is that when the launchpad.net server is heavily loaded, timeouts occur frequently depending on the setting of MaxIdleConnsPerHost.

Can you share the log??

takumin commented 5 years ago

@nishitaniyuki

Tested with the following configuration file: go-apt-mirror.toml (gist)

Synchronization has failed with the previous fix. failed.log (gist)

Synchronization is successful with the new fix. success.log (log)

takumin commented 1 month ago

I'm very sorry for leaving it alone for so long. This PR is old and the current situation is different, so we will close it. thank you very much.