fix Issue 18318 - std.net.curl.download silently ignores non-2xx...

[MartinNowak]

• document, handle, and throw HTTPStatusException on non-2xx responses in other high-level API functions (download, upload, byLineAsync, byChunkAsync)
• similar to how get, post, et.al. work
• still downloads/fetches the error content (e.g. 404 page)

Not fun to work on this at all, as both std.net.curl and std.concurrency are full of issues and pitfalls. Maybe we should deprecate the async functions, and point people to use std.parallelism instead, e.g. with a document example in std.net.curl.

[MartinNowak]

FYI @ikod in case this affects ikod/dlang-requests.

[wilzbach]

FYI @ikod in case this affects ikod/dlang-requests.

dlang-requests is written in native D and uses it's own exceptions, so I don't see how it would be affected.

Maybe we should deprecate the async functions, and point people to use std.parallelism instead,

Imho we should deprecate the entire module and point people to the requests package. It's so much easier to use, native D, well tested, fast and supports async with Vibe.d eventcore out-of-the-box.

[CyberShadow]

Thanks!

Imho we should deprecate the entire module and point people to the requests package.

I quite like std.net.curl for small programs.

• Very simple API, doesn't get simpler than get / download.
• Using it does not slow down the build of the D program.
• Verbose mode is a simple, single switch that enables output useful for diagnosing incorrect requests.
• Supports cookie jars, SSL client (!) certificates, FTP, and other weird things that come with using cURL.

The asynchronous stuff is pretty crazy, though. I never used it and wouldn't recommend it.

[ikod]

FYI @ikod in case this affects ikod/dlang-requests.

Hello Martin,

No, dlang-requests do not use curl, so it can't be affected.

Anyway, thanks for the info!

[MartinNowak]

IMO, simple D out-of-the box examples are enough of an argument to keep std.net.curl. But it's indeed very difficult to use for anything non-trivial, and the async APIs should better be deprecated.

[MartinNowak]

Unittest on Win32 seems to hang, closing PR to save tester resources until that's resolved.

[dlang-bot]

Thanks for your pull request, @MartinNowak!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18318	major	std.net.curl.download silently ignores non-2xx http statuses

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6102"

[wilzbach]

@MartinNowak there are no resources wasted with keeping a PR open for a while. Auto-tester doesn't rerun old PRs anyhow.

[wilzbach]

(rebased)

[wilzbach]

The failure is fetchzip when running dub's testsuite seems to be permanent:

[ERROR] :DUB should have retried download on server error multiple times, but only tried 0 times. command failed

[CyberShadow]

I think this needs a changelog entry, as programs that "worked" previously will now throw. (Programs written before this change might have elected to search for an error message string in the output, or otherwise validate the downloaded data, to detect HTTP errors.)

[andre2007]

As far as I understand dub issue https://github.com/dlang/dub/issues/1530 is related to this pr. Dub has a retry logic if http download is failing. This retry logic only works if HTTPStatusException is thrown. But at the moment instead (e.g. in case of a timeout) CurlException is thrown.

Is my understanding correct, with this pr an HTTPStatusException is thrown also for timeout issues? What is missing in this PR?

[MartinNowak]

Is my understanding correct, with this pr an HTTPStatusException is thrown also for timeout issues?

No, as it's name and docs say it's thrown when an exceptional HTTP status is returned.

[MartinNowak]

rebased and added changelog

[CyberShadow]

@MartinNowak Do you think the custom logic could be replaced with CURLOPT_FAILONERROR, so that we could match the behavior with curl --fail?

[wilzbach]

With this change, test/fetchzip.sh fails in DUB.

Full exception: object.Exception@source/dub/packagesuppliers/registry.d(71): Failed to download package gitcompatibledubpackage from http://localhost:10255/packages/gitcompatibledubpackage/1.0.2.zip

[ERROR] :DUB should have retried download on server error multiple times, but only tried 0 times. command failed

So it looks like DUB retry logic doesn't handle catching these CurlExceptions that well.