Skip to content

Conversation

@ThomasBreuer
Copy link
Contributor

@ThomasBreuer ThomasBreuer commented May 9, 2025

The idea is to specify the maximum number of seconds to wait for a download, via the optional component maxTime in the optional second argument of Download.
The default value is given by the new user preference DownloadMaxTime of the utils package, and the default for that is zero, meaning that there is no timeout.

The methods stored in Download_Methods either use a given nonzero maxTime value or signal failure (the latter holds for the method from the IO package, which does not support timeouts).

(Up to now, I have not managed to test in which situations this timeout really helps. This is why this pull request is marked as draft.)

The idea is to specify the maximum number of seconds to wait for
a download, via the optional component `maxTime` in the optional
second argument of `Download`.
The default value is given by the new user preference `DownloadMaxTime`
of the utils package, and the default for that is zero,
meaning that there is no timeout.

The methods stored in `Download_Methods` either use a given nonzero `maxTime`
value or signal failure (the latter holds for the method from the IO package,
which does not support timeouts).
@ThomasBreuer ThomasBreuer marked this pull request as draft May 9, 2025 12:27
@codecov
Copy link

codecov bot commented May 9, 2025

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.40%. Comparing base (82b58c6) to head (086d7de).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
lib/download.gi 84.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   77.96%   83.40%   +5.43%     
==========================================
  Files          29       29              
  Lines        1829     1693     -136     
==========================================
- Hits         1426     1412      -14     
+ Misses        403      281     -122     
Files with missing lines Coverage Δ
lib/download.gd 100.00% <100.00%> (ø)
lib/download.gi 82.71% <84.00%> (+0.83%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Deal with the situation that the IO package is not available,
and hence the IO based `Download` method cannot be tested.
@cdwensley
Copy link
Collaborator

Is this still "work in progress", or should I be doing something about it?

@ThomasBreuer
Copy link
Contributor Author

@cdwensley

Is this still "work in progress", or should I be doing something about it?

I have to try again to reproduce the error situation, in order to see whether the timeout helps as expected.
At the moment, you need not do anything.

@fingolfin
Copy link
Member

@ThomasBreuer have you considered using https://httpbun.com/delay/5 (replace "5" with any other amount of seconds) for tests, to emulate a server which accepts a connection but then is slow to respond?

It would be really good to get this merged and released so we can start using it, given that RWTH servers had another outage recently

@ThomasBreuer
Copy link
Contributor Author

@fingolfin Thanks for the hint about https://httpbun.com.

The timeouts work as expected, except that old versions of wget ignore timeouts:

  • With wget 1.20.3, the process waits until the delay time is over and then returns success.
  • With wget 1.21.3, the process gives up after the prescribed time.

I am going to exclude wget when a timeout option is set, in the same way as IO's SingleHTTPRequest (which does not support timeouts) is excluded.

- exclude the wget based `Download` method if a `timeout` is given
- add tests for `timeout`
@ThomasBreuer ThomasBreuer marked this pull request as ready for review September 9, 2025 11:33
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@fingolfin
Copy link
Member

@cdwensley would be nice to have this in a release soon (perhaps even in time for the final GAP 4.15.0 release?). See also my PR #84 which makes it easier to produce releases.

@cdwensley cdwensley merged commit 0433b2f into gap-packages:master Sep 10, 2025
6 checks passed
@cdwensley cdwensley mentioned this pull request Sep 10, 2025
@ThomasBreuer ThomasBreuer deleted the TB_Download_timeout branch September 11, 2025 08:14
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.

3 participants