Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle curl_easy_perform() errors #60

Closed
DEGoodmanWilson opened this issue Nov 6, 2015 · 5 comments
Closed

Handle curl_easy_perform() errors #60

DEGoodmanWilson opened this issue Nov 6, 2015 · 5 comments
Milestone

Comments

@DEGoodmanWilson
Copy link
Contributor

Hello! I'd like to be able to detect with a finer-grained resolution errors that prevent Curl from performing a request. It seems the right place to start is to change session.cpp:350 to capture the return value from curl_easy_perform(), and then perhaps add some error fields to the cpr::Response that is returned.

I'm happy to open a PR with this feature added if there were interest in it; and if so, I'd like to get your opinion on the right way to structure this feature.

@whoshuu
Copy link
Contributor

whoshuu commented Nov 6, 2015

This is a great suggestion @DEGoodmanWilson. In my own testing, I've always just set CURL_VERBOSE to 1 to get more information when curl doesn't do what I want it to do.

Now that the library has matured a bit since those days, I can definitely see the value in having a more stable error API.

One approach is to surface curl errors and have the users inspect those errors to figure out what's wrong. I can see how this approach might cause some problems in the future, because I want to leave the frontend of the API as curl-free as possible. Ideally, the design of the interface should allow for the implementation to completely swap out curl for another http framework (looking mostly at Boost.ASIO).

That said, that's a pretty long way off and I think there's enough value here that it's worth doing soon. I think minimally the curl error code could be captured in an integer field in cpr::Response, and potentially the string error from curl_easy_strerror can be captured in cpr::Response as well.

Feel free to throw up a PR and we could take it from there!

@DEGoodmanWilson
Copy link
Contributor Author

Thanks for the quick response! Sounds good. I share your concerns about exposing Curl to the end-user of CPR. Perhaps we could inter-translate into a CPR-owned enum (but continue using the curl-generated message, since that is meant for humans rather than computers anyway?)

What is your feeling on throwing an exception instead of adding the error to cpr::Response? This could be problematic for the aync versions of the HTTP methods. My own inclination is to not throw an exception, but I can see arguments either way.

@whoshuu
Copy link
Contributor

whoshuu commented Nov 7, 2015

I would prefer to keep the library as exception free as possible. Clients could always wrap their own exception mechanisms over a simple error code they can check. Conversely, a client could absorb exceptions and write their own error codes against this, but I think the former is a bit nicer to larger groups of people (looking at you, Google Coding Standards).

I think the approach you're suggesting is reasonable, I'll have to take a closer look at #61 tomorrow when I have some free cycles. If that's good to go, I'll close this issue when that gets merged in.

Does that sound good?

@DEGoodmanWilson
Copy link
Contributor Author

👍 Sounds awesome.

@whoshuu whoshuu added this to the 1.3.0 milestone Dec 10, 2015
@whoshuu
Copy link
Contributor

whoshuu commented Dec 11, 2015

Merged in bdb877c. Thanks for the PR and work!

@whoshuu whoshuu closed this as completed Dec 11, 2015
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

No branches or pull requests

2 participants