feat: add detailed timeout error #170
Conversation
steve-chavez
force-pushed
the
detailed-timeout
branch
from
12bc0a9 to
868f3e4
Compare
When a timeout occurs, now the error message is like: ``` Timeout of 1000 ms reached. Total time: 1000.527000 ms (DNS time: 0.084000 ms, TCP/SSL handshake time: 0.314000 ms, HTTP Request/Response time: 1000.014000 ms) ``` Before it used to be: ``` Timeout was reached ``` Closes supabase#169.
steve-chavez
force-pushed
the
detailed-timeout
branch
from
868f3e4 to
acfdd76
Compare
There was a problem hiding this comment.
Thank you for the PR. All tests pass on my laptop, although I had to increase timeout in time.sleep call within test_http_detailed_timeout() and test_http_get_succeed_with_gt_timeout().
I have couple of suggestions. One is mentioned below. And the second: would it make more sense to add new column error_detail to net._http_response? That way it would be easier to make queries to the table without using pattern matching.
src/core.c
Outdated
|
|
||
| // build the error message | ||
| curl_timeout_msg result = {.msg = {}}; | ||
| sprintf(result.msg, |
There was a problem hiding this comment.
It is safer to call snprintf to make sure that the result string won't be larger than result.msg.
There was a problem hiding this comment.
Good call, done. Also went ahead and moved all this logic to a more proper errors.hs module.
1 similar comment
@za-arthur I thought about that too, but the problem is that users will likely ignore the new detail column and the goal is for them to pay attention to these timings. Additionally, we don't add more details to other error messages, just this one. All the other errors come raw from libcurl. So not much use for the new column. |
Closes #169.
When a timeout occurs, now the error message is like:
Before it used to be: