Report input source errors rather than tokio panics

[katrinafyi]

for example:

cargo run -- https://google.com https://fdsjaifjdsaoi.invalid

before:

thread 'tokio-runtime-worker' (74220) panicked at lychee-bin/src/commands/check.rs:337:18:
response channel closed: SendError { .. }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: Task failed to execute to completion

Caused by:
    task 28 panicked with message "response channel closed: SendError { .. }"

note that the real error isn't printed at all, because it's meant to be returned to main and doesn't panic.

after:

Error: Network error: Connection failed. Check network connectivity and firewall settings (error sending request for url (https://fdsjaifjdsaoi.invalid/))

Caused by:
    0: error sending request for url (https://fdsjaifjdsaoi.invalid/)
    1: client error (Connect)
    2: dns error
    3: failed to lookup address information: Name or service not known

note also that this is not a panic

this was happening due to a subtlety in how errors get raised. the collect_responses task is the one which terminates first on errors, but it was being awaited after other tasks which would panic so its result was ignored.

reporting the error does have to wait for the other tasks to finish or panic (they will panic the next time they try to send through a channel).

[thomas-zahner]

note also that this is not a panic

If I run your provided command on the current version of the PR I get:

thread 'tokio-runtime-worker' (57434) panicked at lychee-bin/src/commands/check.rs:237:18:
cannot send response to queue: SendError { .. }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: Network error: Connection failed. Check network connectivity and firewall settings (error sending request for url (https://fdsjaifjdsaoi.invalid/))

Caused by:
    0: error sending request for url (https://fdsjaifjdsaoi.invalid/)
    1: client error (Connect)
    2: dns error
    3: failed to lookup address information: Name or service not known

This is fine and much better than before. But did you have a version in this PR where you didn't get the panic? Also, it might make sense to add a CLI test, to assure that we don't lose the detailed and useful stack track and details from the error message again in the future.

[katrinafyi]

This is fine and much better than before. But did you have a version in this PR where you didn't get the panic?

Yeah it's a good question. Investigating more, the panic is more likely to be avoided if we race the futures using select!, but it's more complicated and the panic is still shown in some cases. I can still make it appear intermittently using cargo run -- a.md fdsjaifjdsaoi.com and a.md has https links. I wrote a bit in the message of 730ed2a, let me know if you want to keep this or I can revert it.

Also, it might make sense to add a CLI test

Agreed

[thomas-zahner]

Oh wow, 730ed2a looks interesting but also really complicated 😅
Also it seems to introduce some deadlocks as the CI tests got stuck. I would much rather prefer the previous version which was simple in comparison. Panics on invalid user input aren't bad if we get such detailed and good error messages.

[katrinafyi]

FYI the deadlocks were because I was awaiting a FusedFuture after it had already finished, and futures can only be awaited once so it never finished awaiting the second time. But yeah, I agree and I've reverted all the commits.

Idk why the lint is now failing with an error pointing to #2075

[thomas-zahner]

Looks good and seems to work.
Could you also add the CLI test?

[katrinafyi]

Added the cli test in 3c61551 and checked that it fails before this PR :)

[thomas-zahner]

@katrinafyi Thank you! 🚀