@yegor256 Wanna take this for a spin?

@cristiklein would it be possible to do this in the lychee.toml?

accept = [200, 202, 301, 302, 307, 401, timeout]

@cristiklein would it be possible to do this in the lychee.toml?

accept = [200, 202, 301, 302, 307, 401, timeout]

Just tested and it also works via lychee.toml:

$ cat blah.md 
<http://127.0.0.1:8080>
$ cat lychee.toml 
accept = [200, 202, 301, 302, 307, 401, "timeout"]
timeout = 1
$ ./target/debug/lychee --config lychee.toml blah.md
1/1 ━━━━━━━━━━━━━━━━━━━━ Finished extracting links                                                                                                       Issues found in 1 input. Find details below.

[blah.md]:
 [TIMEOUT] http://127.0.0.1:8080/ | Timeout

🔍 1 Total (in 11s) ✅ 0 OK 🚫 0 Errors ⏳ 1 Timeouts

$ echo $?
0

Thank you for the PR. As mentioned over on the issue we should not use such magic/sentinel values.

Reusing --accept might not be ideal in this case. Tough it would be possible, if we wrapped the accept type in a newtype or struct/enum.

It might be simpler and more straight forward to introduce --accept-timeouts both for users and for the implementation. I'm not quite sure what all the implications of this change are. But they might be bigger than the PR in the current state.

For example all output formats still show an erroneous run.
Issues found in 1 input. Find details below. for compact,
error_map for json
Errors in stdin for detailed
### Errors in stdin for markdown

So the timeouts should maybe be marked as success as early as possible, instead of adding is_success_ignoring_timeouts.

Thanks for the initiative @cristiklein. It's nice to see someone taking on that task. 😊

I agree with Thomas here. Those special values can cause hard-to-debug edge cases. I should know because we're still dealing with the aftermath of my ad-hoc decisions from the past. 😬

So, yeah, let's make it explicit.

The lychee config would then look like

accept = [200, 202, 301, 302, 307, 401]

# Accept transient timeouts
accept_timeouts = false

Thanks @mre and @thomas-zahner for the kind words and feedback.

I just force-pushed a new PR, which addresses your feedback.

Below is an example invocation:

$ cat blah.md 
<http://127.0.0.1:8080>
$ cat lychee.toml 
accept = [200, 202, 301, 302, 307, 401]
accept_timeouts = true
timeout = 1
$ ./target/debug/lychee blah.md
1/1 ━━━━━━━━━━━━━━━━━━━━ Finished extracting links                          Issues found in 1 input. Find details below.

[blah.md]:
[TIMEOUT] http://127.0.0.1:8080/ (at 1:2) | Timeout

🔍 1 Total (in 11s) ✅ 0 OK 🚫 0 Errors ⏳ 1 Timeouts

$ echo $?
0

Let me know what you think.

@cristiklein maybe you can update the title and description of the PR, for clarity?

@cristiklein Cool, that looks better. Also great to see the new test 👍

Now we should think about how to display and output such accepted timeouts. Currently, they are still clearly marked as error. This might be unexpected, though maybe the current behaviour is also fine. But my thinking is that if timeouts are accepted timeouts should probably not be shown as errors, but as successes.

So for example the timeouts shouldn't show up in error_map for the json format. The same would be true for all other formats. Although, maybe this would make it harder to spot the timeouts? What are your thoughts @mre @katrinafyi?

I tried it out and I noticed that timeouts are still retried even if --accept-timeouts is given. This makes it take more time than necessary. Is this intentional?

Re output format, I agree that if timeouts are accepted, they shouldn't be printed amongst the errors.

Looking at the related enums, I also think the current enum Status is not the best, because it mixes up two things which I think should be separated: (1) the actual outcome of the request, and (2) how that outcome should be interpreted. This makes it hard to represent cases like a Timeout that should be accepted and we also have to pass the accepted set around a lot. To implement special logic like this PR, it ends up being a lot of post-processing to "reinterpret" the status. If the two concerns were separated, it would be much easier to do this PR and other requests such as rejecting redirects.

But anyway, all that won't be fixed in this PR. In this PR, I guess you would change the lines around where it adds to the error map:

https://github.com/cristiklein/lychee/blob/6a8007ba3d1fb5de95c78c4e60126e824748340d/lychee-bin/src/formatters/stats/response.rs#L93-L106

Hi @katrinafyi . Thanks for looking into the PR.

I tried it out and I noticed that timeouts are still retried even if --accept-timeouts is given. This makes it take more time than necessary. Is this intentional?

Yes, my intention was for --accept-timeout to only change the exit code and nothing else. The user can run lychee --accept-timeouts --max-retries=0 --timeout=1 if they want to minimize execution time.

Re output format, I agree that if timeouts are accepted, they shouldn't be printed amongst the errors.

Just to make sure I understand correctly, do you argue for lychee to suppress outputting [TIMEOUT] and folding that into OKs?

My thinking is the following: This feature will most likely be used in a CI. In that case, the user might still want TIMEOUT in logs, but they want lychee to return 0, so their CI pipeline continues.

Looking at the related enums, I also think the current enum Status is not the best, because it mixes up two things which I think should be separated: (1) the actual outcome of the request, and (2) how that outcome should be interpreted. This makes it hard to represent cases like a Timeout that should be accepted and we also have to pass the accepted set around a lot. To implement special logic like this PR, it ends up being a lot of post-processing to "reinterpret" the status. If the two concerns were separated, it would be much easier to do this PR and other requests such as rejecting redirects.

Just to make sure I understand correctly, are you arguing for moving accept logic to ResponseStats?

But anyway, all that won't be fixed in this PR. In this PR, I guess you would change the lines around where it adds to the error map:

https://github.com/cristiklein/lychee/blob/6a8007ba3d1fb5de95c78c4e60126e824748340d/lychee-bin/src/formatters/stats/response.rs#L93-L106

Just to make sure, you propose timeouts to be added to success_map with --accept-timeouts and error_map without --accept-timeouts? Seems like timeouts are currently not added to any map.

As an alternative, would it make sense to add timeout_map? This would avoid having to pass accept to ResponseStats.add_response_status().

Just to make sure, you propose timeouts to be added to success_map with --accept-timeouts and error_map without --accept-timeouts? Seems like timeouts are currently not added to any map.

If you run

echo https://httpbin.org/delay/2 | cargo run --  - --timeout=1 --max-retries=0 --format json

the timeout are added to error_map (it's included in the is_error() case). I think it would be nice if error_map only contains the errors which would cause link checking to fail. Then, for example, is_success is simply error_map.is_empty() and we don't have to do the big sums.

However, at the moment, printing the timeouts (under the default verbosity) is tied to inclusion in the error_map. It would be nice if this were not the case and that could be done by adding a timeout_map and a separate output section like redirects. But I also defer to @ thomas-zahner and @ mre for whether that is a good idea.

Just to make sure I understand correctly, are you arguing for moving accept logic to ResponseStats?

Sorry, that paragraph was not necessarily directed to you. It's just an early idea and I'm not arguing for anything concrete.

Thanks @katrinafyi for the feedback.

Just to make sure, you propose timeouts to be added to success_map with --accept-timeouts and error_map without --accept-timeouts? Seems like timeouts are currently not added to any map.

If you run

echo https://httpbin.org/delay/2 | cargo run --  - --timeout=1 --max-retries=0 --format json

the timeout are added to error_map (it's included in the is_error() case). I think it would be nice if error_map only contains the errors which would cause link checking to fail. Then, for example, is_success is simply error_map.is_empty() and we don't have to do the big sums.

However, at the moment, printing the timeouts (under the default verbosity) is tied to inclusion in the error_map. It would be nice if this were not the case and that could be done by adding a timeout_map and a separate output section like redirects. But I also defer to @ thomas-zahner and @ mre for whether that is a good idea.

To make the alternative easier to reason about, I went ahead and moved timeouts into timeout_map as a concrete proposal. See 3f9d495

I also simplified is_success and is_success_ignoring_timeouts. is_success now becomes a simple error_map.is_empty() && timeout_map.is_empty(). See 84de85e

Curious whether this direction makes sense conceptually, especially with regard to output verbosity and backward compatibility.

Thanks @thomas-zahner . I accepted all your suggestions and made the tests pass again. PTAL.

I also suggest a few other improvements in 6dc06e9

Sorry, I noticed this too late. I added those suggestions too and fixed the tests accordingly.

@cristiklein Thank you for the work. I also really like the new timeout_map which emerged as part of this PR.

@cristiklein Thank you for the work. I also really like the new timeout_map which emerged as part of this PR.

And thank you for shepherding this PR. I really liked the infrastructure and tests in lychee, and the good discussions we had. It was fun!